| Message ID | 1536949623-23564-4-git-send-email-brijesh.singh@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86_iommu/amd: add interrupt remap support | expand |
On Fri, Sep 14, 2018 at 01:26:58PM -0500, Brijesh Singh wrote: > Currently, the amdvi_validate_dte() assumes that a valid DTE will > always have V=1. This is not true. The V=1 means that bit[127:1] are > valid. A valid DTE can have IV=1 and V=0 (i.e pt=off, intremap=on). "pt" might be a bit confusing here. Now "intel-iommu" device has the "pt" parameter to specify IOMMU DMAR passthrough support. Also the corresponding guest kernel parameter "iommu_pt". So I would suggest to use "page translation" (is this really the term that AMD spec is used after all?) or directly DMAR (DMA remapping). > > Remove the V=1 check from amdvi_validate_dte(), make the caller > responsible to check for V or IV bits. > > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> > Cc: "Michael S. Tsirkin" <mst@redhat.com> > Cc: Paolo Bonzini <pbonzini@redhat.com> > Cc: Richard Henderson <rth@twiddle.net> > Cc: Eduardo Habkost <ehabkost@redhat.com> > Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com> > Cc: Tom Lendacky <Thomas.Lendacky@amd.com> > Cc: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> > --- > hw/i386/amd_iommu.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c > index 1fd669f..225825e 100644 > --- a/hw/i386/amd_iommu.c > +++ b/hw/i386/amd_iommu.c > @@ -807,7 +807,7 @@ static inline uint64_t amdvi_get_perms(uint64_t entry) > AMDVI_DEV_PERM_SHIFT; > } > > -/* a valid entry should have V = 1 and reserved bits honoured */ > +/* validate that reserved bits are honoured */ > static bool amdvi_validate_dte(AMDVIState *s, uint16_t devid, > uint64_t *dte) > { > @@ -820,7 +820,7 @@ static bool amdvi_validate_dte(AMDVIState *s, uint16_t devid, > return false; > } > > - return dte[0] & AMDVI_DEV_VALID; > + return true; > } > > /* get a device table entry given the devid */ > @@ -967,7 +967,8 @@ static void amdvi_do_translate(AMDVIAddressSpace *as, hwaddr addr, > } > > /* devices with V = 0 are not translated */ > - if (!amdvi_get_dte(s, devid, entry)) { > + if (!amdvi_get_dte(s, devid, entry) && > + !(entry[0] & AMDVI_DEV_VALID)) { Here I'm not sure whether you're considering endianess. I think amdvi_get_dte() tried to fix the endianess somehow but I'm not sure it's complete (so entry[0] is special here...): static bool amdvi_get_dte(AMDVIState *s, int devid, uint64_t *entry) { uint32_t offset = devid * AMDVI_DEVTAB_ENTRY_SIZE; if (dma_memory_read(&address_space_memory, s->devtab + offset, entry, AMDVI_DEVTAB_ENTRY_SIZE)) { trace_amdvi_dte_get_fail(s->devtab, offset); /* log error accessing dte */ amdvi_log_devtab_error(s, devid, s->devtab + offset, 0); return false; } *entry = le64_to_cpu(*entry); <----------------------- [1] if (!amdvi_validate_dte(s, devid, entry)) { trace_amdvi_invalid_dte(entry[0]); return false; } return true; } At [1] only one 64bits entry is swapped correctly to cpu endianess, IMHO the rest of the three uint64_t is still using LE. I'm not really sure whether there would be anyone that wants to run the AMD IOMMU on big endian hosts, but I just want to know the goal of this series - do you want to support this scenario? If so, you might need to fixup the places too AFAIU. > goto out; > } > > -- > 2.7.4 > > Regards,
On 9/16/18 11:33 PM, Peter Xu wrote: > On Fri, Sep 14, 2018 at 01:26:58PM -0500, Brijesh Singh wrote: >> Currently, the amdvi_validate_dte() assumes that a valid DTE will >> always have V=1. This is not true. The V=1 means that bit[127:1] are >> valid. A valid DTE can have IV=1 and V=0 (i.e pt=off, intremap=on). > "pt" might be a bit confusing here. Now "intel-iommu" device has the > "pt" parameter to specify IOMMU DMAR passthrough support. Also the > corresponding guest kernel parameter "iommu_pt". So I would suggest > to use "page translation" (is this really the term that AMD spec is > used after all?) or directly DMAR (DMA remapping). I will use "page or address translation" instead of pt to avoid confusions. > >> Remove the V=1 check from amdvi_validate_dte(), make the caller >> responsible to check for V or IV bits. >> >> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> >> Cc: "Michael S. Tsirkin" <mst@redhat.com> >> Cc: Paolo Bonzini <pbonzini@redhat.com> >> Cc: Richard Henderson <rth@twiddle.net> >> Cc: Eduardo Habkost <ehabkost@redhat.com> >> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com> >> Cc: Tom Lendacky <Thomas.Lendacky@amd.com> >> Cc: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> >> --- >> hw/i386/amd_iommu.c | 7 ++++--- >> 1 file changed, 4 insertions(+), 3 deletions(-) >> >> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c >> index 1fd669f..225825e 100644 >> --- a/hw/i386/amd_iommu.c >> +++ b/hw/i386/amd_iommu.c >> @@ -807,7 +807,7 @@ static inline uint64_t amdvi_get_perms(uint64_t entry) >> AMDVI_DEV_PERM_SHIFT; >> } >> >> -/* a valid entry should have V = 1 and reserved bits honoured */ >> +/* validate that reserved bits are honoured */ >> static bool amdvi_validate_dte(AMDVIState *s, uint16_t devid, >> uint64_t *dte) >> { >> @@ -820,7 +820,7 @@ static bool amdvi_validate_dte(AMDVIState *s, uint16_t devid, >> return false; >> } >> >> - return dte[0] & AMDVI_DEV_VALID; >> + return true; >> } >> >> /* get a device table entry given the devid */ >> @@ -967,7 +967,8 @@ static void amdvi_do_translate(AMDVIAddressSpace *as, hwaddr addr, >> } >> >> /* devices with V = 0 are not translated */ >> - if (!amdvi_get_dte(s, devid, entry)) { >> + if (!amdvi_get_dte(s, devid, entry) && >> + !(entry[0] & AMDVI_DEV_VALID)) { > Here I'm not sure whether you're considering endianess. I think > amdvi_get_dte() tried to fix the endianess somehow but I'm not sure > it's complete (so entry[0] is special here...): > > static bool amdvi_get_dte(AMDVIState *s, int devid, uint64_t *entry) > { > uint32_t offset = devid * AMDVI_DEVTAB_ENTRY_SIZE; > > if (dma_memory_read(&address_space_memory, s->devtab + offset, entry, > AMDVI_DEVTAB_ENTRY_SIZE)) { > trace_amdvi_dte_get_fail(s->devtab, offset); > /* log error accessing dte */ > amdvi_log_devtab_error(s, devid, s->devtab + offset, 0); > return false; > } > > *entry = le64_to_cpu(*entry); <----------------------- [1] > if (!amdvi_validate_dte(s, devid, entry)) { > trace_amdvi_invalid_dte(entry[0]); > return false; > } > > return true; > } > > At [1] only one 64bits entry is swapped correctly to cpu endianess, > IMHO the rest of the three uint64_t is still using LE. > > I'm not really sure whether there would be anyone that wants to run > the AMD IOMMU on big endian hosts, but I just want to know the goal of > this series - do you want to support this scenario? If so, you might > need to fixup the places too AFAIU. I had similar question in my mind when I looked at amd-iommu the very first time. I am not sure if anyone is using this device on big endian platform. This series focuses on interrupt remap support only. If needed we can work on different series to add big endian hosts. > >> goto out; >> } >> >> -- >> 2.7.4 >> >> > Regards, >
On Fri, Sep 14, 2018 at 01:26:58PM -0500, Brijesh Singh wrote: > Currently, the amdvi_validate_dte() assumes that a valid DTE will > always have V=1. This is not true. The V=1 means that bit[127:1] are > valid. A valid DTE can have IV=1 and V=0 (i.e pt=off, intremap=on). > > Remove the V=1 check from amdvi_validate_dte(), make the caller > responsible to check for V or IV bits. > > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> > Cc: "Michael S. Tsirkin" <mst@redhat.com> > Cc: Paolo Bonzini <pbonzini@redhat.com> > Cc: Richard Henderson <rth@twiddle.net> > Cc: Eduardo Habkost <ehabkost@redhat.com> > Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com> > Cc: Tom Lendacky <Thomas.Lendacky@amd.com> > Cc: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> > --- > hw/i386/amd_iommu.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c > index 1fd669f..225825e 100644 > --- a/hw/i386/amd_iommu.c > +++ b/hw/i386/amd_iommu.c > @@ -807,7 +807,7 @@ static inline uint64_t amdvi_get_perms(uint64_t entry) > AMDVI_DEV_PERM_SHIFT; > } > > -/* a valid entry should have V = 1 and reserved bits honoured */ > +/* validate that reserved bits are honoured */ > static bool amdvi_validate_dte(AMDVIState *s, uint16_t devid, > uint64_t *dte) > { > @@ -820,7 +820,7 @@ static bool amdvi_validate_dte(AMDVIState *s, uint16_t devid, > return false; > } > > - return dte[0] & AMDVI_DEV_VALID; ^^^^^^^^ [1] > + return true; > } For reference, this is the only caller of amdvi_validate_dte(): /* get a device table entry given the devid */ static bool amdvi_get_dte(AMDVIState *s, int devid, uint64_t *entry) { uint32_t offset = devid * AMDVI_DEVTAB_ENTRY_SIZE; if (dma_memory_read(&address_space_memory, s->devtab + offset, entry, AMDVI_DEVTAB_ENTRY_SIZE)) { trace_amdvi_dte_get_fail(s->devtab, offset); /* log error accessing dte */ amdvi_log_devtab_error(s, devid, s->devtab + offset, 0); return false; } *entry = le64_to_cpu(*entry); if (!amdvi_validate_dte(s, devid, entry)) { /* <--- [2] */ trace_amdvi_invalid_dte(entry[0]); return false; } return true; } and the only caller of amdvi_get_dte() is below: > > /* get a device table entry given the devid */ > @@ -967,7 +967,8 @@ static void amdvi_do_translate(AMDVIAddressSpace *as, hwaddr addr, > } > > /* devices with V = 0 are not translated */ > - if (!amdvi_get_dte(s, devid, entry)) { > + if (!amdvi_get_dte(s, devid, entry) && > + !(entry[0] & AMDVI_DEV_VALID)) { ^^^^^ [3] > goto out; > } This means `dte` at [1] == `entry` at [2] == `entry` at [3]. However, if amdvi_get_dte() returned false, `entry[0]` might be uninitialized. We should check (entry[0] & AMDVI_DEV_VALID) only if amdvi_get_dte() returned true. I assume you meant the following: if (!amdvi_get_dte(s, devid, entry) || !(entry[0] & AMDVI_DEV_VALID)) { goto out; } > > -- > 2.7.4 > >
On 09/17/2018 07:56 AM, Eduardo Habkost wrote: > On Fri, Sep 14, 2018 at 01:26:58PM -0500, Brijesh Singh wrote: >> Currently, the amdvi_validate_dte() assumes that a valid DTE will >> always have V=1. This is not true. The V=1 means that bit[127:1] are >> valid. A valid DTE can have IV=1 and V=0 (i.e pt=off, intremap=on). >> >> Remove the V=1 check from amdvi_validate_dte(), make the caller >> responsible to check for V or IV bits. >> >> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> >> Cc: "Michael S. Tsirkin" <mst@redhat.com> >> Cc: Paolo Bonzini <pbonzini@redhat.com> >> Cc: Richard Henderson <rth@twiddle.net> >> Cc: Eduardo Habkost <ehabkost@redhat.com> >> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com> >> Cc: Tom Lendacky <Thomas.Lendacky@amd.com> >> Cc: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> >> --- >> hw/i386/amd_iommu.c | 7 ++++--- >> 1 file changed, 4 insertions(+), 3 deletions(-) >> >> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c >> index 1fd669f..225825e 100644 >> --- a/hw/i386/amd_iommu.c >> +++ b/hw/i386/amd_iommu.c >> @@ -807,7 +807,7 @@ static inline uint64_t amdvi_get_perms(uint64_t entry) >> AMDVI_DEV_PERM_SHIFT; >> } >> >> -/* a valid entry should have V = 1 and reserved bits honoured */ >> +/* validate that reserved bits are honoured */ >> static bool amdvi_validate_dte(AMDVIState *s, uint16_t devid, >> uint64_t *dte) >> { >> @@ -820,7 +820,7 @@ static bool amdvi_validate_dte(AMDVIState *s, uint16_t devid, >> return false; >> } >> >> - return dte[0] & AMDVI_DEV_VALID; > > ^^^^^^^^ [1] > >> + return true; >> } > > For reference, this is the only caller of amdvi_validate_dte(): > > /* get a device table entry given the devid */ > static bool amdvi_get_dte(AMDVIState *s, int devid, uint64_t *entry) > { > uint32_t offset = devid * AMDVI_DEVTAB_ENTRY_SIZE; > > if (dma_memory_read(&address_space_memory, s->devtab + offset, entry, > AMDVI_DEVTAB_ENTRY_SIZE)) { > trace_amdvi_dte_get_fail(s->devtab, offset); > /* log error accessing dte */ > amdvi_log_devtab_error(s, devid, s->devtab + offset, 0); > return false; > } > > *entry = le64_to_cpu(*entry); > if (!amdvi_validate_dte(s, devid, entry)) { /* <--- [2] */ > trace_amdvi_invalid_dte(entry[0]); > return false; > } > > return true; > } > > and the only caller of amdvi_get_dte() is below: > >> >> /* get a device table entry given the devid */ >> @@ -967,7 +967,8 @@ static void amdvi_do_translate(AMDVIAddressSpace *as, hwaddr addr, >> } >> >> /* devices with V = 0 are not translated */ >> - if (!amdvi_get_dte(s, devid, entry)) { >> + if (!amdvi_get_dte(s, devid, entry) && >> + !(entry[0] & AMDVI_DEV_VALID)) { > ^^^^^ [3] > >> goto out; >> } > > This means `dte` at [1] == `entry` at [2] == `entry` at [3]. > > However, if amdvi_get_dte() returned false, `entry[0]` might be > uninitialized. We should check (entry[0] & AMDVI_DEV_VALID) only > if amdvi_get_dte() returned true. I assume you meant the > following: > > if (!amdvi_get_dte(s, devid, entry) || > !(entry[0] & AMDVI_DEV_VALID)) { > goto out; > } > Ah good catch. Yes we should check the valid bit only if we are able to get a valid dte. thanks
On Mon, Sep 17, 2018 at 06:51:24AM -0500, Brijesh Singh wrote: > > > On 9/16/18 11:33 PM, Peter Xu wrote: > > On Fri, Sep 14, 2018 at 01:26:58PM -0500, Brijesh Singh wrote: > >> Currently, the amdvi_validate_dte() assumes that a valid DTE will > >> always have V=1. This is not true. The V=1 means that bit[127:1] are > >> valid. A valid DTE can have IV=1 and V=0 (i.e pt=off, intremap=on). > > "pt" might be a bit confusing here. Now "intel-iommu" device has the > > "pt" parameter to specify IOMMU DMAR passthrough support. Also the > > corresponding guest kernel parameter "iommu_pt". So I would suggest > > to use "page translation" (is this really the term that AMD spec is > > used after all?) or directly DMAR (DMA remapping). > > I will use "page or address translation" instead of pt to avoid confusions. > > > > >> Remove the V=1 check from amdvi_validate_dte(), make the caller > >> responsible to check for V or IV bits. > >> > >> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> > >> Cc: "Michael S. Tsirkin" <mst@redhat.com> > >> Cc: Paolo Bonzini <pbonzini@redhat.com> > >> Cc: Richard Henderson <rth@twiddle.net> > >> Cc: Eduardo Habkost <ehabkost@redhat.com> > >> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com> > >> Cc: Tom Lendacky <Thomas.Lendacky@amd.com> > >> Cc: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> > >> --- > >> hw/i386/amd_iommu.c | 7 ++++--- > >> 1 file changed, 4 insertions(+), 3 deletions(-) > >> > >> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c > >> index 1fd669f..225825e 100644 > >> --- a/hw/i386/amd_iommu.c > >> +++ b/hw/i386/amd_iommu.c > >> @@ -807,7 +807,7 @@ static inline uint64_t amdvi_get_perms(uint64_t entry) > >> AMDVI_DEV_PERM_SHIFT; > >> } > >> > >> -/* a valid entry should have V = 1 and reserved bits honoured */ > >> +/* validate that reserved bits are honoured */ > >> static bool amdvi_validate_dte(AMDVIState *s, uint16_t devid, > >> uint64_t *dte) > >> { > >> @@ -820,7 +820,7 @@ static bool amdvi_validate_dte(AMDVIState *s, uint16_t devid, > >> return false; > >> } > >> > >> - return dte[0] & AMDVI_DEV_VALID; > >> + return true; > >> } > >> > >> /* get a device table entry given the devid */ > >> @@ -967,7 +967,8 @@ static void amdvi_do_translate(AMDVIAddressSpace *as, hwaddr addr, > >> } > >> > >> /* devices with V = 0 are not translated */ > >> - if (!amdvi_get_dte(s, devid, entry)) { > >> + if (!amdvi_get_dte(s, devid, entry) && > >> + !(entry[0] & AMDVI_DEV_VALID)) { > > Here I'm not sure whether you're considering endianess. I think > > amdvi_get_dte() tried to fix the endianess somehow but I'm not sure > > it's complete (so entry[0] is special here...): > > > > static bool amdvi_get_dte(AMDVIState *s, int devid, uint64_t *entry) > > { > > uint32_t offset = devid * AMDVI_DEVTAB_ENTRY_SIZE; > > > > if (dma_memory_read(&address_space_memory, s->devtab + offset, entry, > > AMDVI_DEVTAB_ENTRY_SIZE)) { > > trace_amdvi_dte_get_fail(s->devtab, offset); > > /* log error accessing dte */ > > amdvi_log_devtab_error(s, devid, s->devtab + offset, 0); > > return false; > > } > > > > *entry = le64_to_cpu(*entry); <----------------------- [1] > > if (!amdvi_validate_dte(s, devid, entry)) { > > trace_amdvi_invalid_dte(entry[0]); > > return false; > > } > > > > return true; > > } > > > > At [1] only one 64bits entry is swapped correctly to cpu endianess, > > IMHO the rest of the three uint64_t is still using LE. > > > > I'm not really sure whether there would be anyone that wants to run > > the AMD IOMMU on big endian hosts, but I just want to know the goal of > > this series - do you want to support this scenario? If so, you might > > need to fixup the places too AFAIU. > > I had similar question in my mind when I looked at amd-iommu the very > first time. I am not sure if anyone is using this device on big endian > platform. This series focuses on interrupt remap support only. If needed > we can work on different series to add big endian hosts. It's fine to me then. Though I would at least suggest you detect the endianess in realize() of the IOMMU object and fault there if you detected that you're running on a BE host, prompting something like "AMD IOMMU is only allowed to run on little-endian hosts". Regards,
diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c index 1fd669f..225825e 100644 --- a/hw/i386/amd_iommu.c +++ b/hw/i386/amd_iommu.c @@ -807,7 +807,7 @@ static inline uint64_t amdvi_get_perms(uint64_t entry) AMDVI_DEV_PERM_SHIFT; } -/* a valid entry should have V = 1 and reserved bits honoured */ +/* validate that reserved bits are honoured */ static bool amdvi_validate_dte(AMDVIState *s, uint16_t devid, uint64_t *dte) { @@ -820,7 +820,7 @@ static bool amdvi_validate_dte(AMDVIState *s, uint16_t devid, return false; } - return dte[0] & AMDVI_DEV_VALID; + return true; } /* get a device table entry given the devid */ @@ -967,7 +967,8 @@ static void amdvi_do_translate(AMDVIAddressSpace *as, hwaddr addr, } /* devices with V = 0 are not translated */ - if (!amdvi_get_dte(s, devid, entry)) { + if (!amdvi_get_dte(s, devid, entry) && + !(entry[0] & AMDVI_DEV_VALID)) { goto out; }
Currently, the amdvi_validate_dte() assumes that a valid DTE will always have V=1. This is not true. The V=1 means that bit[127:1] are valid. A valid DTE can have IV=1 and V=0 (i.e pt=off, intremap=on). Remove the V=1 check from amdvi_validate_dte(), make the caller responsible to check for V or IV bits. Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> Cc: "Michael S. Tsirkin" <mst@redhat.com> Cc: Paolo Bonzini <pbonzini@redhat.com> Cc: Richard Henderson <rth@twiddle.net> Cc: Eduardo Habkost <ehabkost@redhat.com> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com> Cc: Tom Lendacky <Thomas.Lendacky@amd.com> Cc: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> --- hw/i386/amd_iommu.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)