Re: [Xen-devel] [PATCH 3 of 6] amd iommu: Fix iommu page size encoding when page order > 0

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 02.11.11 at 15:52, Wei Wang2 <wei.wang2@xxxxxxx> wrote:
> On Wednesday 02 November 2011 14:46:24 Jan Beulich wrote:
>> >>> On 25.10.11 at 15:07, Wei Wang <wei.wang2@xxxxxxx> wrote:
>> >
>> > # HG changeset patch
>> > # User Wei Wang <wei.wang2@xxxxxxx>
>> > # Date 1319472692 -7200
>> > # Node ID 18088bd3e8f6c16b7aef3d8652f2b9878117fcd5
>> > # Parent  0d17087f9e495c1b9eb43bc0f6a21319097f3043
>> > amd iommu: Fix iommu page size encoding when page order > 0
>> >
>> > Signed-off-by: Wei Wang <wei.wang2@xxxxxxx>
>> >
>> > diff -r 0d17087f9e49 -r 18088bd3e8f6
>> > xen/drivers/passthrough/amd/iommu_map.c ---
>> > a/xen/drivers/passthrough/amd/iommu_map.c  Mon Oct 24 18:11:29 2011 +0200
>> > +++ b/xen/drivers/passthrough/amd/iommu_map.c      Mon Oct 24 18:11:32 2011
>> > +0200 @@ -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 )
>> > -    {
>> > -        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);
>> >  }
>> >
>> >  void amd_iommu_flush_pages(struct domain *d,
>> > diff -r 0d17087f9e49 -r 18088bd3e8f6
>> > xen/include/asm-x86/hvm/svm/amd-iommu-defs.h ---
>> > a/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h     Mon Oct 24 18:11:29 2011
>> > +0200 +++ b/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h   Mon Oct 24
>> > 18:11:32 2011 +0200
>> > @@ -407,4 +407,6 @@
>> >  #define INT_REMAP_ENTRY_VECTOR_MASK     0x00FF0000
>> >  #define INT_REMAP_ENTRY_VECTOR_SHIFT    16
>> >
>> > +#define INV_IOMMU_ALL_PAGES_ADDRESS     0x7FFFFFFFFFFFFULL
>>
>> ((1ULL << 51) - 1)
>>
>> would make this much easier to understand. But - doe the IOMMU really
>> have its limit at 51 bits (as opposed to the architectural limit of 52 in
>> the CPU's MMU)?
> 
> Maybe the name is confusing. Actually it is a special frame number not an 
> address. Bit 63 = 0 indicates all page frames should be invalidated. Or I 
> should use INV_IOMMU_ALL_PAGES_PFN  instead. Anyway, I will fix that in the 

Yes, if this is a PFN, then the name should say so. And then it becomes
all the more relevant to express this as a decremented shift expression
(as it depends on PAGE_SHIFT).

Jan

> next version. Thanks for all your comments. Other issues will also be fixed 
> in the new version.
> Thanks,
> Wei  
> 
>> Jan
>>
>> > +
>> >  #endif /* _ASM_X86_64_AMD_IOMMU_DEFS_H */
>> >
>> >
>> > _______________________________________________
>> > Xen-devel mailing list
>> > Xen-devel@xxxxxxxxxxxxxxxxxxx 
>> > http://lists.xensource.com/xen-devel 




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