Re: [RESEND PATCH 1/5] vtd: Ensure root entry is updated consistently

[Teddy Astie <teddy.astie@xxxxxxxxxx>]

Le 04/08/2026 à 18:00, Jan Beulich a écrit :
> On 29.07.2026 12:05, Teddy Astie wrote:
> > --- a/xen/drivers/passthrough/vtd/iommu.c
> > +++ b/xen/drivers/passthrough/vtd/iommu.c
> > @@ -281,13 +281,13 @@ void free_pgtable_maddr(u64 maddr)
> >   /* context entry handling */
> >   static u64 bus_to_context_maddr(struct vtd_iommu *iommu, u8 bus)
> >   {
> > -    struct root_entry *root, *root_entries;
> > +    struct root_entry root, *root_entries;
> >       u64 maddr;
> >   
> >       ASSERT(spin_is_locked(&iommu->lock));
> >       root_entries = (struct root_entry 
> > *)map_vtd_domain_page(iommu->root_maddr);
> > -    root = &root_entries[bus];
> > -    if ( !root_present(*root) )
> > +    root.val = ACCESS_ONCE(root_entries[bus].val);
> I'm pretty concerned about this: You're reading only half of the entry here,
> and you're writing only half of it further down. The other half is reserved
> right now, but there's not even a comment being added to this effect. (Yet
> even with a comment, I'd still be concerned, just not as much.)
The idea is to match the original logic while making it always compile 
correctly. As we don't interact with the other part of the root entry 
(reserved, or upper context table with Scalable-Mode).
Ideally, it should be written using a bitfield structure instead, but 
it's a much larger change.
> > @@ -295,11 +295,12 @@ static u64 bus_to_context_maddr(struct vtd_iommu *iommu, 
> > u8 bus)
> >               unmap_vtd_domain_page(root_entries);
> >               return 0;
> >           }
> > -        set_root_value(*root, maddr);
> > -        set_root_present(*root);
> > -        iommu_sync_cache(root, sizeof(struct root_entry));
> > +        set_root_value(root, maddr);
> > +        set_root_present(root);
> > +        ACCESS_ONCE(root_entries[bus].val) = root.val;
> > +        iommu_sync_cache(&root_entries[bus], sizeof(struct root_entry));
> sizeof(<expression>) please in favor of sizeof(<type>), whenever possible.
>
> >       }
> > -    maddr = (u64) get_context_addr(*root);
> > +    maddr = (u64) get_context_addr(root);
> While there, drop the pointless casts, thus getting rid of two style issues
> as well?
Fixed locally.

> Jan
Teddy

Attachment: OpenPGP_0x660FA9D102CBCFD0.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature