Re: Re: [PATCH 2 of 5] amd iommu: Fix iommu page size encoding when page order > 0

[Wei Wang2 <wei.wang2@amd.com>]

On Thursday 03 November 2011 13:52:18 Jan Beulich wrote:
> >>> On 03.11.11 at 13:49, "Jan Beulich" <JBeulich@suse.com> wrote:
> >>>> On 03.11.11 at 11:13, Wei Wang <wei.wang2@amd.com> wrote:
> >>
> >> # HG changeset patch
> >> # User Wei Wang <wei.wang2@amd.com>
> >> # Date 1320314617 -3600
> >> # Node ID d0c38cb215cd96e01de27eadf5ec0a5e711de448
> >> # Parent  d422e3cf7976c76c57fc2d455b784d0fcc24d06b
> >> amd iommu: Fix iommu page size encoding when page order > 0
> >>
> >> Signed-off-by: Wei Wang <wei.wang2@amd.com>
> >>
> >> diff -r d422e3cf7976 -r d0c38cb215cd
> >> xen/drivers/passthrough/amd/iommu_map.c ---
> >> a/xen/drivers/passthrough/amd/iommu_map.c	Thu Nov 03 11:02:17 2011 +0100
> >> +++ b/xen/drivers/passthrough/amd/iommu_map.c	Thu Nov 03 11:03:37 2011
> >> +0100 @@ -77,23 +77,24 @@ static void invalidate_iommu_pages(struc
> >>  {
> >>      u64 addr_lo, addr_hi;
> >>      u32 cmd[4], entry;
> >> -    u64 mask = 0;
> >>      int sflag = 0, pde = 0;
> >>
> >> +    ASSERT ( order == 0 || order == 9 || order == 18 );
> >> +
> >> +    /* All pages associated with the domainID are invalidated */
> >> +    if ( order || (io_addr == INV_IOMMU_ALL_PAGES_ADDRESS ) )
> >> +    {
> >> +        sflag = 1;
> >> +        pde = 1;
> >> +    }
> >> +
> >>      /* If sflag == 1, the size of the invalidate command is determined
> >>       by the first zero bit in the address starting from Address[12] */
> >> -    if ( order == 9 || order == 18 )
> >> +    if ( order )
> >>      {
> >> -        mask = ((1ULL << (order - 1)) - 1) << PAGE_SHIFT;
> >> -        io_addr |= mask;
> >> -        sflag = 1;
> >> -    }
> >> -
> >> -    /* All pages associated with the domainID are invalidated */
> >> -    else if ( io_addr == 0x7FFFFFFFFFFFF000ULL )
> >
> > Note the difference between this and your adjusted definition
> > of INV_IOMMU_ALL_PAGES_ADDRESS. I don't think this is
> > correct (or else your patch description should say why).
> >
> >> -    {
> >> -        sflag = 1;
> >> -        pde = 1;
> >> +        u64 mask = 1ULL << (order - 1 + PAGE_SHIFT);
> >> +        io_addr &= ~mask;
> >> +        io_addr |= mask - 1;
> >>      }
> >>
> >>      addr_lo = io_addr & DMA_32BIT_MASK;
> >> @@ -917,7 +918,7 @@ static void _amd_iommu_flush_pages(struc
> >>
> >>  void amd_iommu_flush_all_pages(struct domain *d)
> >>  {
> >> -    _amd_iommu_flush_pages(d, 0x7FFFFFFFFFFFFULL, 0);
> >> +    _amd_iommu_flush_pages(d, INV_IOMMU_ALL_PAGES_ADDRESS, 0);
> >
> > Same here, for a slightly different reason.
> >
> > Jan
> >
> >>  }
> >>
> >>  void amd_iommu_flush_pages(struct domain *d,
> >> diff -r d422e3cf7976 -r d0c38cb215cd
> >> xen/include/asm-x86/hvm/svm/amd-iommu-defs.h ---
> >> a/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h	Thu Nov 03 11:02:17 2011
> >> +0100 +++ b/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h	Thu Nov 03
> >> 11:03:37 2011 +0100
> >> @@ -407,4 +407,6 @@
> >>  #define INT_REMAP_ENTRY_VECTOR_MASK     0x00FF0000
> >>  #define INT_REMAP_ENTRY_VECTOR_SHIFT    16
> >>
> >> +#define INV_IOMMU_ALL_PAGES_ADDRESS      0x7FFFFFFFFFFFFFFFULL
> >> +
>
> Oh, additionally, this still isn't being expressed by a shift expression,
> which makes it still badly readable (one has to count F-s in order to
> know what this actually represents).
Well, this is a copy from the spec.. How about using  (1<<63) -1?
Thanks,
Wei

> Jan
>
> >>  #endif /* _ASM_X86_64_AMD_IOMMU_DEFS_H */
> >
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xensource.com
> > http://lists.xensource.com/xen-devel