Re: [Qemu-devel] [PATCH v2 4/8] x86_iommu/amd: Prepare for interrupt remap support
On 09/17/2018 01:06 PM, Eduardo Habkost wrote:
...#define TYPE_AMD_IOMMU_DEVICE "amd-iommu"
#define AMD_IOMMU_DEVICE(obj)\
@@ -278,6 +288,9 @@ typedef struct AMDVIState {
/* IOTLB */
GHashTable *iotlb;
+
+/* Interrupt remapping */
+bool intr_enabled;
Why do you need this field if the same info is already available
at AMDVIState::iommu::intr_supported?
Again this is to be consistent with intel-iommu structure which has this
fields. Having said that I should be able to access the
AMDVIState::iommu::intr_supported and remove this new field.
intel-iommu seems to need the field because intr_enabled can
change at runtime at vtd_handle_gcmd_ire(). Does amd-iommu have
anything equivalent?
Actually there is no MMIO control register writes to detect whether
the guest OS enabled the interrupt remap feature. I believe in case of
intel we are able to intercept some register writes which gives info
whether the IR is enabled by guest. In amd-iommu, only way to check if
whether guest OS has enabled the IR is by looking at DTE IV bit - this
lookup is done when we get request to remap the interrupt.
In summary, intr_enabled = iommu->intr_supported hence I also do
not see much value for having this field in AMDVIState.
-Brijesh
Re: [Qemu-devel] [PATCH v2 4/8] x86_iommu/amd: Prepare for interrupt remap support
On Mon, Sep 17, 2018 at 11:00:46AM -0500, Brijesh Singh wrote:
>
>
> On 09/17/2018 08:49 AM, Eduardo Habkost wrote:
> > Hi,
> >
> > I couldn't review the whole patch yet, but I have some comments
> > below:
> >
> > On Fri, Sep 14, 2018 at 01:26:59PM -0500, Brijesh Singh wrote:
> > > Register the interrupt remapping callback and read/write ops for the
> > > amd-iommu-ir memory region.
> > >
> > > amd-iommu-ir is set to higher priority to ensure that this region won't
> > > be masked out by other memory regions.
> > >
> > > While at it, add a overlapping amd-iommu region with higher priority
> > > and update address space name to include the devfn.
> > >
> > > Cc: "Michael S. Tsirkin"
> > > Cc: Paolo Bonzini
> > > Cc: Richard Henderson
> > > Cc: Eduardo Habkost
> > > Cc: Marcel Apfelbaum
> > > Cc: Tom Lendacky
> > > Cc: Suravee Suthikulpanit
> > > Signed-off-by: Brijesh Singh
> > > ---
> > > hw/i386/amd_iommu.c | 140
> > > ---
> > > hw/i386/amd_iommu.h | 17 ++-
> > > hw/i386/trace-events | 5 ++
> > > 3 files changed, 154 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
> > > index 225825e..b15962b 100644
> > > --- a/hw/i386/amd_iommu.c
> > > +++ b/hw/i386/amd_iommu.c
> > > @@ -26,6 +26,7 @@
> > > #include "amd_iommu.h"
> > > #include "qapi/error.h"
> > > #include "qemu/error-report.h"
> > > +#include "hw/i386/apic_internal.h"
> > > #include "trace.h"
> > > /* used AMD-Vi MMIO registers */
> > > @@ -56,6 +57,7 @@ struct AMDVIAddressSpace {
> > > uint8_t devfn; /* device function
> > > */
> > > AMDVIState *iommu_state;/* AMDVI - one per machine
> > > */
> > > IOMMUMemoryRegion iommu;/* Device's address translation region
> > > */
> > > +MemoryRegion root; /* AMDVI Root memory map region */
> > > MemoryRegion iommu_ir; /* Device's interrupt remapping region
> > > */
> > > AddressSpace as;/* device's corresponding address space
> > > */
> > > };
> > > @@ -1027,10 +1029,104 @@ static IOMMUTLBEntry
> > > amdvi_translate(IOMMUMemoryRegion *iommu, hwaddr addr,
> > > return ret;
> > > }
> > > +/* Interrupt remapping for MSI/MSI-X entry */
> > > +static int amdvi_int_remap_msi(AMDVIState *iommu,
> > > + MSIMessage *origin,
> > > + MSIMessage *translated,
> > > + uint16_t sid)
> > > +{
> > > +assert(origin && translated);
> > > +
> > > +trace_amdvi_ir_remap_msi_req(origin->address, origin->data, sid);
> > > +
> > > +if (!iommu || !iommu->intr_enabled) {
> > > +memcpy(translated, origin, sizeof(*origin));
> > > +goto out;
> > > +}
> > > +
> > > +if (origin->address & AMDVI_MSI_ADDR_HI_MASK) {
> > > +trace_amdvi_err("MSI address high 32 bits non-zero when "
> > > +"Interrupt Remapping enabled.");
> > > +return -AMDVI_IR_ERR;
> > > +}
> > > +
> > > +if ((origin->address & AMDVI_MSI_ADDR_LO_MASK) !=
> > > APIC_DEFAULT_ADDRESS) {
> > > +trace_amdvi_err("MSI is not from IOAPIC.");
> > > +return -AMDVI_IR_ERR;
> > > +}
> > > +
> > > +out:
> > > +trace_amdvi_ir_remap_msi(origin->address, origin->data,
> > > + translated->address, translated->data);
> > > +return 0;
> > > +}
> > > +
> > > +static int amdvi_int_remap(X86IOMMUState *iommu,
> > > + MSIMessage *origin,
> > > + MSIMessage *translated,
> > > + uint16_t sid)
> > > +{
> > > +return amdvi_int_remap_msi(AMD_IOMMU_DEVICE(iommu), origin,
> > > + translated, sid);
> > > +}
> > > +
> > > +static MemTxResult amdvi_mem_ir_write(void *opaque, hwaddr addr,
> > > + uint64_t value, unsigned size,
> > > + MemTxAttrs attrs)
> > > +{
> > > +int ret;
> > > +MSIMessage from = { 0, 0 }, to = { 0, 0 };
> > > +uint16_t sid = AMDVI_IOAPIC_SB_DEVID;
> > > +
> > > +from.address = (uint64_t) addr + AMDVI_INT_ADDR_FIRST;
> > > +from.data = (uint32_t) value;
> > > +
> > > +trace_amdvi_mem_ir_write_req(addr, value, size);
> > > +
> > > +if (!attrs.unspecified) {
> > > +/* We have explicit Source ID */
> > > +sid = attrs.requester_id;
> > > +}
> > > +
> > > +ret = amdvi_int_remap_msi(opaque, , , sid);
> > > +if (ret < 0) {
> > > +/* TODO: report error */
> >
> > How do you plan to address this TODO item?
> >
> > > +/* Drop the interrupt */
> >
> > What does this comment mean? Is this also a TODO item?
>
>
> As per the specs, if we are not able to remap the interrupts then we
> should be log the events so that if needed guest OS can access the
Re: [Qemu-devel] [PATCH v2 4/8] x86_iommu/amd: Prepare for interrupt remap support
On 09/17/2018 08:49 AM, Eduardo Habkost wrote:
Hi,
I couldn't review the whole patch yet, but I have some comments
below:
On Fri, Sep 14, 2018 at 01:26:59PM -0500, Brijesh Singh wrote:
Register the interrupt remapping callback and read/write ops for the
amd-iommu-ir memory region.
amd-iommu-ir is set to higher priority to ensure that this region won't
be masked out by other memory regions.
While at it, add a overlapping amd-iommu region with higher priority
and update address space name to include the devfn.
Cc: "Michael S. Tsirkin"
Cc: Paolo Bonzini
Cc: Richard Henderson
Cc: Eduardo Habkost
Cc: Marcel Apfelbaum
Cc: Tom Lendacky
Cc: Suravee Suthikulpanit
Signed-off-by: Brijesh Singh
---
hw/i386/amd_iommu.c | 140 ---
hw/i386/amd_iommu.h | 17 ++-
hw/i386/trace-events | 5 ++
3 files changed, 154 insertions(+), 8 deletions(-)
diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index 225825e..b15962b 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -26,6 +26,7 @@
#include "amd_iommu.h"
#include "qapi/error.h"
#include "qemu/error-report.h"
+#include "hw/i386/apic_internal.h"
#include "trace.h"
/* used AMD-Vi MMIO registers */
@@ -56,6 +57,7 @@ struct AMDVIAddressSpace {
uint8_t devfn; /* device function */
AMDVIState *iommu_state;/* AMDVI - one per machine */
IOMMUMemoryRegion iommu;/* Device's address translation region */
+MemoryRegion root; /* AMDVI Root memory map region */
MemoryRegion iommu_ir; /* Device's interrupt remapping region */
AddressSpace as;/* device's corresponding address space */
};
@@ -1027,10 +1029,104 @@ static IOMMUTLBEntry amdvi_translate(IOMMUMemoryRegion
*iommu, hwaddr addr,
return ret;
}
+/* Interrupt remapping for MSI/MSI-X entry */
+static int amdvi_int_remap_msi(AMDVIState *iommu,
+ MSIMessage *origin,
+ MSIMessage *translated,
+ uint16_t sid)
+{
+assert(origin && translated);
+
+trace_amdvi_ir_remap_msi_req(origin->address, origin->data, sid);
+
+if (!iommu || !iommu->intr_enabled) {
+memcpy(translated, origin, sizeof(*origin));
+goto out;
+}
+
+if (origin->address & AMDVI_MSI_ADDR_HI_MASK) {
+trace_amdvi_err("MSI address high 32 bits non-zero when "
+"Interrupt Remapping enabled.");
+return -AMDVI_IR_ERR;
+}
+
+if ((origin->address & AMDVI_MSI_ADDR_LO_MASK) != APIC_DEFAULT_ADDRESS) {
+trace_amdvi_err("MSI is not from IOAPIC.");
+return -AMDVI_IR_ERR;
+}
+
+out:
+trace_amdvi_ir_remap_msi(origin->address, origin->data,
+ translated->address, translated->data);
+return 0;
+}
+
+static int amdvi_int_remap(X86IOMMUState *iommu,
+ MSIMessage *origin,
+ MSIMessage *translated,
+ uint16_t sid)
+{
+return amdvi_int_remap_msi(AMD_IOMMU_DEVICE(iommu), origin,
+ translated, sid);
+}
+
+static MemTxResult amdvi_mem_ir_write(void *opaque, hwaddr addr,
+ uint64_t value, unsigned size,
+ MemTxAttrs attrs)
+{
+int ret;
+MSIMessage from = { 0, 0 }, to = { 0, 0 };
+uint16_t sid = AMDVI_IOAPIC_SB_DEVID;
+
+from.address = (uint64_t) addr + AMDVI_INT_ADDR_FIRST;
+from.data = (uint32_t) value;
+
+trace_amdvi_mem_ir_write_req(addr, value, size);
+
+if (!attrs.unspecified) {
+/* We have explicit Source ID */
+sid = attrs.requester_id;
+}
+
+ret = amdvi_int_remap_msi(opaque, , , sid);
+if (ret < 0) {
+/* TODO: report error */
How do you plan to address this TODO item?
+/* Drop the interrupt */
What does this comment mean? Is this also a TODO item?
As per the specs, if we are not able to remap the interrupts then we
should be log the events so that if needed guest OS can access the log
events and make some decisions. I have not implemented this yet.
I still need to understand how all these things works before
attempting to emulate this part of code.
I have to see what can be done in addition to log to handle the
cases where we failed to remap. For now, I just added a comment so that
it reminds us to revisit it.
+return MEMTX_ERROR;
+}
+
+apic_get_class()->send_msi();
+
+trace_amdvi_mem_ir_write(to.address, to.data);
+return MEMTX_OK;
+}
+
+static MemTxResult amdvi_mem_ir_read(void *opaque, hwaddr addr,
+ uint64_t *data, unsigned size,
+ MemTxAttrs attrs)
+{
+return MEMTX_OK;
+}
+
+static const MemoryRegionOps amdvi_ir_ops = {
+.read_with_attrs = amdvi_mem_ir_read,
+
Re: [Qemu-devel] [PATCH v2 4/8] x86_iommu/amd: Prepare for interrupt remap support
Hi,
I couldn't review the whole patch yet, but I have some comments
below:
On Fri, Sep 14, 2018 at 01:26:59PM -0500, Brijesh Singh wrote:
> Register the interrupt remapping callback and read/write ops for the
> amd-iommu-ir memory region.
>
> amd-iommu-ir is set to higher priority to ensure that this region won't
> be masked out by other memory regions.
>
> While at it, add a overlapping amd-iommu region with higher priority
> and update address space name to include the devfn.
>
> Cc: "Michael S. Tsirkin"
> Cc: Paolo Bonzini
> Cc: Richard Henderson
> Cc: Eduardo Habkost
> Cc: Marcel Apfelbaum
> Cc: Tom Lendacky
> Cc: Suravee Suthikulpanit
> Signed-off-by: Brijesh Singh
> ---
> hw/i386/amd_iommu.c | 140
> ---
> hw/i386/amd_iommu.h | 17 ++-
> hw/i386/trace-events | 5 ++
> 3 files changed, 154 insertions(+), 8 deletions(-)
>
> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
> index 225825e..b15962b 100644
> --- a/hw/i386/amd_iommu.c
> +++ b/hw/i386/amd_iommu.c
> @@ -26,6 +26,7 @@
> #include "amd_iommu.h"
> #include "qapi/error.h"
> #include "qemu/error-report.h"
> +#include "hw/i386/apic_internal.h"
> #include "trace.h"
>
> /* used AMD-Vi MMIO registers */
> @@ -56,6 +57,7 @@ struct AMDVIAddressSpace {
> uint8_t devfn; /* device function */
> AMDVIState *iommu_state;/* AMDVI - one per machine */
> IOMMUMemoryRegion iommu;/* Device's address translation region */
> +MemoryRegion root; /* AMDVI Root memory map region */
> MemoryRegion iommu_ir; /* Device's interrupt remapping region */
> AddressSpace as;/* device's corresponding address space */
> };
> @@ -1027,10 +1029,104 @@ static IOMMUTLBEntry
> amdvi_translate(IOMMUMemoryRegion *iommu, hwaddr addr,
> return ret;
> }
>
> +/* Interrupt remapping for MSI/MSI-X entry */
> +static int amdvi_int_remap_msi(AMDVIState *iommu,
> + MSIMessage *origin,
> + MSIMessage *translated,
> + uint16_t sid)
> +{
> +assert(origin && translated);
> +
> +trace_amdvi_ir_remap_msi_req(origin->address, origin->data, sid);
> +
> +if (!iommu || !iommu->intr_enabled) {
> +memcpy(translated, origin, sizeof(*origin));
> +goto out;
> +}
> +
> +if (origin->address & AMDVI_MSI_ADDR_HI_MASK) {
> +trace_amdvi_err("MSI address high 32 bits non-zero when "
> +"Interrupt Remapping enabled.");
> +return -AMDVI_IR_ERR;
> +}
> +
> +if ((origin->address & AMDVI_MSI_ADDR_LO_MASK) != APIC_DEFAULT_ADDRESS) {
> +trace_amdvi_err("MSI is not from IOAPIC.");
> +return -AMDVI_IR_ERR;
> +}
> +
> +out:
> +trace_amdvi_ir_remap_msi(origin->address, origin->data,
> + translated->address, translated->data);
> +return 0;
> +}
> +
> +static int amdvi_int_remap(X86IOMMUState *iommu,
> + MSIMessage *origin,
> + MSIMessage *translated,
> + uint16_t sid)
> +{
> +return amdvi_int_remap_msi(AMD_IOMMU_DEVICE(iommu), origin,
> + translated, sid);
> +}
> +
> +static MemTxResult amdvi_mem_ir_write(void *opaque, hwaddr addr,
> + uint64_t value, unsigned size,
> + MemTxAttrs attrs)
> +{
> +int ret;
> +MSIMessage from = { 0, 0 }, to = { 0, 0 };
> +uint16_t sid = AMDVI_IOAPIC_SB_DEVID;
> +
> +from.address = (uint64_t) addr + AMDVI_INT_ADDR_FIRST;
> +from.data = (uint32_t) value;
> +
> +trace_amdvi_mem_ir_write_req(addr, value, size);
> +
> +if (!attrs.unspecified) {
> +/* We have explicit Source ID */
> +sid = attrs.requester_id;
> +}
> +
> +ret = amdvi_int_remap_msi(opaque, , , sid);
> +if (ret < 0) {
> +/* TODO: report error */
How do you plan to address this TODO item?
> +/* Drop the interrupt */
What does this comment mean? Is this also a TODO item?
> +return MEMTX_ERROR;
> +}
> +
> +apic_get_class()->send_msi();
> +
> +trace_amdvi_mem_ir_write(to.address, to.data);
> +return MEMTX_OK;
> +}
> +
> +static MemTxResult amdvi_mem_ir_read(void *opaque, hwaddr addr,
> + uint64_t *data, unsigned size,
> + MemTxAttrs attrs)
> +{
> +return MEMTX_OK;
> +}
> +
> +static const MemoryRegionOps amdvi_ir_ops = {
> +.read_with_attrs = amdvi_mem_ir_read,
> +.write_with_attrs = amdvi_mem_ir_write,
> +.endianness = DEVICE_LITTLE_ENDIAN,
> +.impl = {
> +.min_access_size = 4,
> +.max_access_size = 4,
> +},
> +.valid = {
> +.min_access_size = 4,
> +.max_access_size = 4,
> +}
> +};
> +
> static
[Qemu-devel] [PATCH v2 4/8] x86_iommu/amd: Prepare for interrupt remap support
Register the interrupt remapping callback and read/write ops for the
amd-iommu-ir memory region.
amd-iommu-ir is set to higher priority to ensure that this region won't
be masked out by other memory regions.
While at it, add a overlapping amd-iommu region with higher priority
and update address space name to include the devfn.
Cc: "Michael S. Tsirkin"
Cc: Paolo Bonzini
Cc: Richard Henderson
Cc: Eduardo Habkost
Cc: Marcel Apfelbaum
Cc: Tom Lendacky
Cc: Suravee Suthikulpanit
Signed-off-by: Brijesh Singh
---
hw/i386/amd_iommu.c | 140 ---
hw/i386/amd_iommu.h | 17 ++-
hw/i386/trace-events | 5 ++
3 files changed, 154 insertions(+), 8 deletions(-)
diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index 225825e..b15962b 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -26,6 +26,7 @@
#include "amd_iommu.h"
#include "qapi/error.h"
#include "qemu/error-report.h"
+#include "hw/i386/apic_internal.h"
#include "trace.h"
/* used AMD-Vi MMIO registers */
@@ -56,6 +57,7 @@ struct AMDVIAddressSpace {
uint8_t devfn; /* device function */
AMDVIState *iommu_state;/* AMDVI - one per machine */
IOMMUMemoryRegion iommu;/* Device's address translation region */
+MemoryRegion root; /* AMDVI Root memory map region */
MemoryRegion iommu_ir; /* Device's interrupt remapping region */
AddressSpace as;/* device's corresponding address space */
};
@@ -1027,10 +1029,104 @@ static IOMMUTLBEntry amdvi_translate(IOMMUMemoryRegion
*iommu, hwaddr addr,
return ret;
}
+/* Interrupt remapping for MSI/MSI-X entry */
+static int amdvi_int_remap_msi(AMDVIState *iommu,
+ MSIMessage *origin,
+ MSIMessage *translated,
+ uint16_t sid)
+{
+assert(origin && translated);
+
+trace_amdvi_ir_remap_msi_req(origin->address, origin->data, sid);
+
+if (!iommu || !iommu->intr_enabled) {
+memcpy(translated, origin, sizeof(*origin));
+goto out;
+}
+
+if (origin->address & AMDVI_MSI_ADDR_HI_MASK) {
+trace_amdvi_err("MSI address high 32 bits non-zero when "
+"Interrupt Remapping enabled.");
+return -AMDVI_IR_ERR;
+}
+
+if ((origin->address & AMDVI_MSI_ADDR_LO_MASK) != APIC_DEFAULT_ADDRESS) {
+trace_amdvi_err("MSI is not from IOAPIC.");
+return -AMDVI_IR_ERR;
+}
+
+out:
+trace_amdvi_ir_remap_msi(origin->address, origin->data,
+ translated->address, translated->data);
+return 0;
+}
+
+static int amdvi_int_remap(X86IOMMUState *iommu,
+ MSIMessage *origin,
+ MSIMessage *translated,
+ uint16_t sid)
+{
+return amdvi_int_remap_msi(AMD_IOMMU_DEVICE(iommu), origin,
+ translated, sid);
+}
+
+static MemTxResult amdvi_mem_ir_write(void *opaque, hwaddr addr,
+ uint64_t value, unsigned size,
+ MemTxAttrs attrs)
+{
+int ret;
+MSIMessage from = { 0, 0 }, to = { 0, 0 };
+uint16_t sid = AMDVI_IOAPIC_SB_DEVID;
+
+from.address = (uint64_t) addr + AMDVI_INT_ADDR_FIRST;
+from.data = (uint32_t) value;
+
+trace_amdvi_mem_ir_write_req(addr, value, size);
+
+if (!attrs.unspecified) {
+/* We have explicit Source ID */
+sid = attrs.requester_id;
+}
+
+ret = amdvi_int_remap_msi(opaque, , , sid);
+if (ret < 0) {
+/* TODO: report error */
+/* Drop the interrupt */
+return MEMTX_ERROR;
+}
+
+apic_get_class()->send_msi();
+
+trace_amdvi_mem_ir_write(to.address, to.data);
+return MEMTX_OK;
+}
+
+static MemTxResult amdvi_mem_ir_read(void *opaque, hwaddr addr,
+ uint64_t *data, unsigned size,
+ MemTxAttrs attrs)
+{
+return MEMTX_OK;
+}
+
+static const MemoryRegionOps amdvi_ir_ops = {
+.read_with_attrs = amdvi_mem_ir_read,
+.write_with_attrs = amdvi_mem_ir_write,
+.endianness = DEVICE_LITTLE_ENDIAN,
+.impl = {
+.min_access_size = 4,
+.max_access_size = 4,
+},
+.valid = {
+.min_access_size = 4,
+.max_access_size = 4,
+}
+};
+
static AddressSpace *amdvi_host_dma_iommu(PCIBus *bus, void *opaque, int devfn)
{
+char name[128];
AMDVIState *s = opaque;
-AMDVIAddressSpace **iommu_as;
+AMDVIAddressSpace **iommu_as, *amdvi_dev_as;
int bus_num = pci_bus_num(bus);
iommu_as = s->address_spaces[bus_num];
@@ -1043,19 +1139,46 @@ static AddressSpace *amdvi_host_dma_iommu(PCIBus *bus,
void *opaque, int devfn)
/* set up AMD-Vi region */
if (!iommu_as[devfn]) {
+snprintf(name, sizeof(name), "amd_iommu_devfn_%d", devfn);
+
