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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 03.11.11 at 13:49, "Jan Beulich" <JBeulich@xxxxxxxx> wrote:
>>>> On 03.11.11 at 11:13, Wei Wang <wei.wang2@xxxxxxx> wrote:
>> # HG changeset patch
>> # User Wei Wang <wei.wang2@xxxxxxx>
>> # 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@xxxxxxx>
>> 
>> 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).

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