Mailing List Archive

Hi,

Looks like you've sorted out shooting down old users of a p2m table.
hap_write_p2m_entry still isn't right, though:

> @@ -834,38 +864,81 @@ static void
> hap_write_p2m_entry(struct vcpu *v, unsigned long gfn, l1_pgentry_t *p,
> mfn_t table_mfn, l1_pgentry_t new, unsigned int level)
> {
> - uint32_t old_flags;
> + struct domain *d = v->domain;
> + uint32_t old_flags = l1e_get_flags(*p);

You have moved this read outside the hap_lock. Please put it back.

> + p2m_type_t op2mt = p2m_flags_to_type(old_flags);
>
> - hap_lock(v->domain);
> + /* We know always use the host p2m here, regardless if the vcpu
> + * is in host or guest mode. The vcpu can be in guest mode by
> + * a hypercall which passes a domain and chooses mostly the first
> + * vcpu.
> + * XXX This is the reason why this function can not be used re-used
> + * for updating the nestedp2m. Otherwise, hypercalls would randomly
> + * operate on host p2m and nested p2m.
> + */
> + if ( nestedhvm_enabled(d)
> + && p2m_is_valid(op2mt) )
> + {
> + if ( l1e_get_intpte(new) != l1e_get_intpte(*p) ) {
> + p2m_type_t np2mt = p2m_flags_to_type(l1e_get_flags(new));
>
> - old_flags = l1e_get_flags(*p);
> + /* Skip flush on vram tracking or XP mode in Win7 hang
> + * very early in the virtual BIOS (long before the bootloader
> + * runs), otherwise. VRAM tracking happens so often that
> + * flushing and fixing the nestedp2m doesn't let XP mode
> + * proceed to boot.
> + */
> + if ( !((op2mt == p2m_ram_rw && np2mt == p2m_ram_logdirty)
> + || (op2mt == p2m_ram_logdirty && np2mt == p2m_ram_rw)) )

That's not safe. If the MFN has changed, you _have_ to flush, even if
you're replacing a logdirty entry with a r/w one. And if you're
replacing a r/w entry with a logdirty one, you _have_ to flush or
logdirty doesn't work correctly. If that case is too slow then you
should batch the flushes somehow, not just skip them.

Cheers,

Tim.

> + {
> + /* This GFN -> MFN is going to get removed. */
> + /* XXX There is a more efficient way to do that
> + * but it works for now.
> + * Note, p2m_flush_nestedp2m calls hap_lock() internally.
> + */
> + p2m_flush_nestedp2m(d);
> + }
> + }
> + }
> +
> + hap_lock(d);
> +
> safe_write_pte(p, new);
> if ( (old_flags & _PAGE_PRESENT)
> && (level == 1 || (level == 2 && (old_flags & _PAGE_PSE))) )
> - flush_tlb_mask(&v->domain->domain_dirty_cpumask);
> + flush_tlb_mask(&d->domain_dirty_cpumask);
>
> #if CONFIG_PAGING_LEVELS == 3
> /* install P2M in monitor table for PAE Xen */
> if ( level == 3 )
> /* We have written to the p2m l3: need to sync the per-vcpu
> * copies of it in the monitor tables */
> - p2m_install_entry_in_monitors(v->domain, (l3_pgentry_t *)p);
> + p2m_install_entry_in_monitors(d, (l3_pgentry_t *)p);
> #endif
>
> - hap_unlock(v->domain);
> + hap_unlock(d);
> }

--
Tim Deegan <Tim.Deegan@citrix.com>
Principal Software Engineer, Xen Platform Team
Citrix Systems UK Ltd. (Company #02937203, SL9 0BG)

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

This is the new version. I fixed the open items from Tim's last review.

--
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

On 03/31/11 17:25, Christoph Egger wrote:
>
> This is the new version. I fixed the open items from Tim's last review.

Sorry, I mistakenly resent an older version and noticed it just now.
This time this is the latest version.

Christoph


--
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85689 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

At 16:48 +0100 on 05 Apr (1302022090), Christoph Egger wrote:
> On 03/31/11 17:25, Christoph Egger wrote:
> >
> > This is the new version. I fixed the open items from Tim's last review.
>
> Sorry, I mistakenly resent an older version and noticed it just now.
> This time this is the latest version.

Thank you. I have applied the full series, which should appear as
23157--23168 in the staging tree soon. I had to forward-port a few
small things, and I also fixed up one last race condition in the remote
shootdowns, as 23170:86f87da1445a, so please check that I havent broken
anything in your tests.

Cheers,

Tim.

--
Tim Deegan <Tim.Deegan@citrix.com>
Principal Software Engineer, Xen Platform Team
Citrix Systems UK Ltd. (Company #02937203, SL9 0BG)

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 04/06/11 12:29, Tim Deegan wrote:
> At 16:48 +0100 on 05 Apr (1302022090), Christoph Egger wrote:
>> On 03/31/11 17:25, Christoph Egger wrote:
>>>
>>> This is the new version. I fixed the open items from Tim's last review.
>>
>> Sorry, I mistakenly resent an older version and noticed it just now.
>> This time this is the latest version.
>
> Thank you. I have applied the full series, which should appear as
> 23157--23168 in the staging tree soon. I had to forward-port a few
> small things, and I also fixed up one last race condition in the remote
> shootdowns, as 23170:86f87da1445a, so please check that I havent broken
> anything in your tests.

Thank you for applying the patch series. xentrace was broken again but
that wasn't you. I already submitted a fix for this.

Christoph


--
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85689 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

>>> On 05.04.11 at 17:48, Christoph Egger <Christoph.Egger@amd.com> wrote:
>diff -r cfde4384be14 -r 28809c365861 xen/include/asm-x86/domain.h
>--- a/xen/include/asm-x86/domain.h
>+++ b/xen/include/asm-x86/domain.h
>...
>@@ -225,6 +227,7 @@ struct paging_vcpu {
> #define MAX_CPUID_INPUT 40
> typedef xen_domctl_cpuid_t cpuid_input_t;
>
>+#define MAX_NESTEDP2M 10
> struct p2m_domain;
> struct time_scale {
> int shift;
>@@ -258,6 +261,12 @@ struct arch_domain
> struct paging_domain paging;
> struct p2m_domain *p2m;
>
>+ /* nestedhvm: translate l2 guest physical to host physical */
>+ struct p2m_domain *nested_p2m[MAX_NESTEDP2M];
>+ spinlock_t nested_p2m_lock;
>+ int nested_p2m_locker;
>+ const char *nested_p2m_function;
>+
> /* NB. protected by d->event_lock and by irq_desc[irq].lock */
> int *irq_pirq;
> int *pirq_irq;

Was there a specific reason to add this to struct arch_domain
instead of struct hvm_domain? I.e. can any pf these fields be
used on pv (or idle) domains?

Thanks, Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 04/29/11 11:03, Jan Beulich wrote:
>>>> On 05.04.11 at 17:48, Christoph Egger<Christoph.Egger@amd.com> wrote:
>> diff -r cfde4384be14 -r 28809c365861 xen/include/asm-x86/domain.h
>> --- a/xen/include/asm-x86/domain.h
>> +++ b/xen/include/asm-x86/domain.h
>> ...
>> @@ -225,6 +227,7 @@ struct paging_vcpu {
>> #define MAX_CPUID_INPUT 40
>> typedef xen_domctl_cpuid_t cpuid_input_t;
>>
>> +#define MAX_NESTEDP2M 10
>> struct p2m_domain;
>> struct time_scale {
>> int shift;
>> @@ -258,6 +261,12 @@ struct arch_domain
>> struct paging_domain paging;
>> struct p2m_domain *p2m;
>>
>> + /* nestedhvm: translate l2 guest physical to host physical */
>> + struct p2m_domain *nested_p2m[MAX_NESTEDP2M];
>> + spinlock_t nested_p2m_lock;
>> + int nested_p2m_locker;
>> + const char *nested_p2m_function;
>> +
>> /* NB. protected by d->event_lock and by irq_desc[irq].lock */
>> int *irq_pirq;
>> int *pirq_irq;
>
> Was there a specific reason to add this to struct arch_domain
> instead of struct hvm_domain? I.e. can any pf these fields be
> used on pv (or idle) domains?

The reason is that there is already a 'struct p2m_domain *p2m' field.
If that can be moved to struct hvm_domain then nested_p2m can
definitely move over to there, too.

Christoph

--
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85689 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

>>> On 29.04.11 at 11:09, Christoph Egger <Christoph.Egger@amd.com> wrote:
> On 04/29/11 11:03, Jan Beulich wrote:
>>>>> On 05.04.11 at 17:48, Christoph Egger<Christoph.Egger@amd.com> wrote:
>>> diff -r cfde4384be14 -r 28809c365861 xen/include/asm-x86/domain.h
>>> --- a/xen/include/asm-x86/domain.h
>>> +++ b/xen/include/asm-x86/domain.h
>>> ...
>>> @@ -225,6 +227,7 @@ struct paging_vcpu {
>>> #define MAX_CPUID_INPUT 40
>>> typedef xen_domctl_cpuid_t cpuid_input_t;
>>>
>>> +#define MAX_NESTEDP2M 10
>>> struct p2m_domain;
>>> struct time_scale {
>>> int shift;
>>> @@ -258,6 +261,12 @@ struct arch_domain
>>> struct paging_domain paging;
>>> struct p2m_domain *p2m;
>>>
>>> + /* nestedhvm: translate l2 guest physical to host physical */
>>> + struct p2m_domain *nested_p2m[MAX_NESTEDP2M];
>>> + spinlock_t nested_p2m_lock;
>>> + int nested_p2m_locker;
>>> + const char *nested_p2m_function;
>>> +
>>> /* NB. protected by d->event_lock and by irq_desc[irq].lock */
>>> int *irq_pirq;
>>> int *pirq_irq;
>>
>> Was there a specific reason to add this to struct arch_domain
>> instead of struct hvm_domain? I.e. can any pf these fields be
>> used on pv (or idle) domains?
>
> The reason is that there is already a 'struct p2m_domain *p2m' field.
> If that can be moved to struct hvm_domain then nested_p2m can
> definitely move over to there, too.

No, I don't think these are connected - a pv domain can still require
a p2m (e.g. for the iommu), but I would have thought that the
nesting stuff doesn't apply there.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel