subject:"\[PATCH v12 09\/13\] virtio\-iommu\: Implement fault reporting"

Re: [PATCH v12 09/13] virtio-iommu: Implement fault reporting

2020-01-15 Thread Auger Eric

Hi Peter,
On 1/15/20 4:04 PM, Peter Xu wrote:
> On Wed, Jan 15, 2020 at 02:12:20PM +0100, Auger Eric wrote:
 +static void virtio_iommu_report_fault(VirtIOIOMMU *viommu, uint8_t reason,
 +  int flags, uint32_t endpoint,
 +  uint64_t address)
 +{
> 
> [...]
> 
 +if (iov_size(elem->in_sg, elem->in_num) < sizeof(fault)) {
 +virtio_error(vdev, "error buffer of wrong size");
 +virtqueue_detach_element(vq, elem, 0);
 +g_free(elem);
 +continue;
>>>
>>> If virtio_error(), should we stop rather than continue?
>> My understanding is the buffer just popped had a wrong size so it is not
>> usable. We skip it we try to use another one if any. Does it make sense?
> 
> I'm not very familiar to virtio, but I see that virtio_error marks
> vdev->broken to true.  If with that iiuc the next virtqueue_pop() will
> fail directly (see the first call to virtio_device_disabled in
> virtqueue_pop).  Then I don't see why retry any more...
You're right. I will fix it.

Thanks

Eric
> 
> Thanks,
>

Re: [PATCH v12 09/13] virtio-iommu: Implement fault reporting

2020-01-15 Thread Peter Xu

On Wed, Jan 15, 2020 at 02:12:20PM +0100, Auger Eric wrote:
> >> +static void virtio_iommu_report_fault(VirtIOIOMMU *viommu, uint8_t reason,
> >> +  int flags, uint32_t endpoint,
> >> +  uint64_t address)
> >> +{

[...]

> >> +if (iov_size(elem->in_sg, elem->in_num) < sizeof(fault)) {
> >> +virtio_error(vdev, "error buffer of wrong size");
> >> +virtqueue_detach_element(vq, elem, 0);
> >> +g_free(elem);
> >> +continue;
> > 
> > If virtio_error(), should we stop rather than continue?
> My understanding is the buffer just popped had a wrong size so it is not
> usable. We skip it we try to use another one if any. Does it make sense?

I'm not very familiar to virtio, but I see that virtio_error marks
vdev->broken to true.  If with that iiuc the next virtqueue_pop() will
fail directly (see the first call to virtio_device_disabled in
virtqueue_pop).  Then I don't see why retry any more...

Thanks,

-- 
Peter Xu

Re: [PATCH v12 09/13] virtio-iommu: Implement fault reporting

2020-01-15 Thread Auger Eric

Hi Peter,

On 1/14/20 8:04 PM, Peter Xu wrote:
> On Thu, Jan 09, 2020 at 03:43:15PM +0100, Eric Auger wrote:
>> The event queue allows to report asynchronous errors.
>> The translate function now injects faults when relevant.
>>
>> Signed-off-by: Eric Auger 
>>
>> ---
>>
>> v11 -> v12:
>> - reporting the addr associated with the fault and set the
>>   VIRTIO_IOMMU_FAULT_F_ADDRESS flag.
>> - added cpu_to_le
>>
>> v10 -> v11:
>> - change a virtio_error into an error_report_once
>>   (no buffer available for output faults)
>> ---
>>  hw/virtio/trace-events   |  1 +
>>  hw/virtio/virtio-iommu.c | 73 +---
>>  2 files changed, 69 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
>> index 095aa8b509..e83500bee9 100644
>> --- a/hw/virtio/trace-events
>> +++ b/hw/virtio/trace-events
>> @@ -72,3 +72,4 @@ virtio_iommu_put_endpoint(uint32_t ep_id) "Free 
>> endpoint=%d"
>>  virtio_iommu_get_domain(uint32_t domain_id) "Alloc domain=%d"
>>  virtio_iommu_put_domain(uint32_t domain_id) "Free domain=%d"
>>  virtio_iommu_translate_out(uint64_t virt_addr, uint64_t phys_addr, uint32_t 
>> sid) "0x%"PRIx64" -> 0x%"PRIx64 " for sid=%d"
>> +virtio_iommu_report_fault(uint8_t reason, uint32_t flags, uint32_t 
>> endpoint, uint64_t addr) "FAULT reason=%d flags=%d endpoint=%d address 
>> =0x%"PRIx64
>> diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
>> index d192bcb505..09193970ee 100644
>> --- a/hw/virtio/virtio-iommu.c
>> +++ b/hw/virtio/virtio-iommu.c
>> @@ -477,6 +477,51 @@ out:
>>  }
>>  }
>>  
>> +static void virtio_iommu_report_fault(VirtIOIOMMU *viommu, uint8_t reason,
>> +  int flags, uint32_t endpoint,
>> +  uint64_t address)
>> +{
>> +VirtIODevice *vdev = >parent_obj;
>> +VirtQueue *vq = viommu->event_vq;
>> +struct virtio_iommu_fault fault;
>> +VirtQueueElement *elem;
>> +size_t sz;
>> +
>> +memset(, 0, sizeof(fault));
>> +fault.reason = reason;
>> +fault.flags = cpu_to_le32(flags);
>> +fault.endpoint = cpu_to_le32(endpoint);
>> +fault.address = cpu_to_le64(address);
>> +
>> +for (;;) {
>> +elem = virtqueue_pop(vq, sizeof(VirtQueueElement));
>> +
>> +if (!elem) {
>> +error_report_once(
>> +"no buffer available in event queue to report event");
> 
> (Should this also be a guest issue?  IIRC you are still using a
>  mixture of both qemu_log_mask and error_report()... I'll stop
>  commenting on this, assuming that you prefer both ways to be used...)

I've just removed the qemu_log_mask in virtio_iommu_unmap(). So now you
should not find any qemu_log_mask anymore. Sorry for the oversight.
> 
>> +return;
>> +}
>> +
>> +if (iov_size(elem->in_sg, elem->in_num) < sizeof(fault)) {
>> +virtio_error(vdev, "error buffer of wrong size");
>> +virtqueue_detach_element(vq, elem, 0);
>> +g_free(elem);
>> +continue;
> 
> If virtio_error(), should we stop rather than continue?
My understanding is the buffer just popped had a wrong size so it is not
usable. We skip it we try to use another one if any. Does it make sense?

Thanks

Eric
> 
>> +}
>> +break;
>> +}
>> +/* we have a buffer to fill in */
>> +sz = iov_from_buf(elem->in_sg, elem->in_num, 0,
>> +  , sizeof(fault));
>> +assert(sz == sizeof(fault));
>> +
>> +trace_virtio_iommu_report_fault(reason, flags, endpoint, address);
>> +virtqueue_push(vq, elem, sz);
>> +virtio_notify(vdev, vq);
>> +g_free(elem);
>> +
>> +}
>> +
>>  static IOMMUTLBEntry virtio_iommu_translate(IOMMUMemoryRegion *mr, hwaddr 
>> addr,
>>  IOMMUAccessFlags flag,
>>  int iommu_idx)
>> @@ -485,9 +530,10 @@ static IOMMUTLBEntry 
>> virtio_iommu_translate(IOMMUMemoryRegion *mr, hwaddr addr,
>>  VirtIOIOMMUInterval interval, *mapping_key;
>>  VirtIOIOMMUMapping *mapping_value;
>>  VirtIOIOMMU *s = sdev->viommu;
>> +bool read_fault, write_fault;
>>  VirtIOIOMMUEndpoint *ep;
>> +uint32_t sid, flags;
>>  bool bypass_allowed;
>> -uint32_t sid;
>>  bool found;
>>  
>>  interval.low = addr;
>> @@ -513,6 +559,9 @@ static IOMMUTLBEntry 
>> virtio_iommu_translate(IOMMUMemoryRegion *mr, hwaddr addr,
>>  if (!ep) {
>>  if (!bypass_allowed) {
>>  error_report_once("%s sid=%d is not known!!", __func__, sid);
>> +virtio_iommu_report_fault(s, VIRTIO_IOMMU_FAULT_R_UNKNOWN,
>> +  VIRTIO_IOMMU_FAULT_F_ADDRESS,
>> +  sid, addr);
>>  } else {
>>  entry.perm = flag;
>>  }
>> @@ -524,6 +573,9 @@ static IOMMUTLBEntry 
>> virtio_iommu_translate(IOMMUMemoryRegion *mr, hwaddr addr,
>>

Re: [PATCH v12 09/13] virtio-iommu: Implement fault reporting

2020-01-14 Thread Peter Xu

On Thu, Jan 09, 2020 at 03:43:15PM +0100, Eric Auger wrote:
> The event queue allows to report asynchronous errors.
> The translate function now injects faults when relevant.
> 
> Signed-off-by: Eric Auger 
> 
> ---
> 
> v11 -> v12:
> - reporting the addr associated with the fault and set the
>   VIRTIO_IOMMU_FAULT_F_ADDRESS flag.
> - added cpu_to_le
> 
> v10 -> v11:
> - change a virtio_error into an error_report_once
>   (no buffer available for output faults)
> ---
>  hw/virtio/trace-events   |  1 +
>  hw/virtio/virtio-iommu.c | 73 +---
>  2 files changed, 69 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
> index 095aa8b509..e83500bee9 100644
> --- a/hw/virtio/trace-events
> +++ b/hw/virtio/trace-events
> @@ -72,3 +72,4 @@ virtio_iommu_put_endpoint(uint32_t ep_id) "Free endpoint=%d"
>  virtio_iommu_get_domain(uint32_t domain_id) "Alloc domain=%d"
>  virtio_iommu_put_domain(uint32_t domain_id) "Free domain=%d"
>  virtio_iommu_translate_out(uint64_t virt_addr, uint64_t phys_addr, uint32_t 
> sid) "0x%"PRIx64" -> 0x%"PRIx64 " for sid=%d"
> +virtio_iommu_report_fault(uint8_t reason, uint32_t flags, uint32_t endpoint, 
> uint64_t addr) "FAULT reason=%d flags=%d endpoint=%d address =0x%"PRIx64
> diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
> index d192bcb505..09193970ee 100644
> --- a/hw/virtio/virtio-iommu.c
> +++ b/hw/virtio/virtio-iommu.c
> @@ -477,6 +477,51 @@ out:
>  }
>  }
>  
> +static void virtio_iommu_report_fault(VirtIOIOMMU *viommu, uint8_t reason,
> +  int flags, uint32_t endpoint,
> +  uint64_t address)
> +{
> +VirtIODevice *vdev = >parent_obj;
> +VirtQueue *vq = viommu->event_vq;
> +struct virtio_iommu_fault fault;
> +VirtQueueElement *elem;
> +size_t sz;
> +
> +memset(, 0, sizeof(fault));
> +fault.reason = reason;
> +fault.flags = cpu_to_le32(flags);
> +fault.endpoint = cpu_to_le32(endpoint);
> +fault.address = cpu_to_le64(address);
> +
> +for (;;) {
> +elem = virtqueue_pop(vq, sizeof(VirtQueueElement));
> +
> +if (!elem) {
> +error_report_once(
> +"no buffer available in event queue to report event");

(Should this also be a guest issue?  IIRC you are still using a
 mixture of both qemu_log_mask and error_report()... I'll stop
 commenting on this, assuming that you prefer both ways to be used...)

> +return;
> +}
> +
> +if (iov_size(elem->in_sg, elem->in_num) < sizeof(fault)) {
> +virtio_error(vdev, "error buffer of wrong size");
> +virtqueue_detach_element(vq, elem, 0);
> +g_free(elem);
> +continue;

If virtio_error(), should we stop rather than continue?

> +}
> +break;
> +}
> +/* we have a buffer to fill in */
> +sz = iov_from_buf(elem->in_sg, elem->in_num, 0,
> +  , sizeof(fault));
> +assert(sz == sizeof(fault));
> +
> +trace_virtio_iommu_report_fault(reason, flags, endpoint, address);
> +virtqueue_push(vq, elem, sz);
> +virtio_notify(vdev, vq);
> +g_free(elem);
> +
> +}
> +
>  static IOMMUTLBEntry virtio_iommu_translate(IOMMUMemoryRegion *mr, hwaddr 
> addr,
>  IOMMUAccessFlags flag,
>  int iommu_idx)
> @@ -485,9 +530,10 @@ static IOMMUTLBEntry 
> virtio_iommu_translate(IOMMUMemoryRegion *mr, hwaddr addr,
>  VirtIOIOMMUInterval interval, *mapping_key;
>  VirtIOIOMMUMapping *mapping_value;
>  VirtIOIOMMU *s = sdev->viommu;
> +bool read_fault, write_fault;
>  VirtIOIOMMUEndpoint *ep;
> +uint32_t sid, flags;
>  bool bypass_allowed;
> -uint32_t sid;
>  bool found;
>  
>  interval.low = addr;
> @@ -513,6 +559,9 @@ static IOMMUTLBEntry 
> virtio_iommu_translate(IOMMUMemoryRegion *mr, hwaddr addr,
>  if (!ep) {
>  if (!bypass_allowed) {
>  error_report_once("%s sid=%d is not known!!", __func__, sid);
> +virtio_iommu_report_fault(s, VIRTIO_IOMMU_FAULT_R_UNKNOWN,
> +  VIRTIO_IOMMU_FAULT_F_ADDRESS,
> +  sid, addr);
>  } else {
>  entry.perm = flag;
>  }
> @@ -524,6 +573,9 @@ static IOMMUTLBEntry 
> virtio_iommu_translate(IOMMUMemoryRegion *mr, hwaddr addr,
>  error_report_once("%s %02x:%02x.%01x not attached to any domain",
>__func__, PCI_BUS_NUM(sid),
>PCI_SLOT(sid), PCI_FUNC(sid));
> +virtio_iommu_report_fault(s, VIRTIO_IOMMU_FAULT_R_DOMAIN,
> +  VIRTIO_IOMMU_FAULT_F_ADDRESS,
> +  sid, addr);
>  } else {
>  entry.perm = flag;
>  }
> @@ -536,15

[PATCH v12 09/13] virtio-iommu: Implement fault reporting

2020-01-09 Thread Eric Auger

The event queue allows to report asynchronous errors.
The translate function now injects faults when relevant.

Signed-off-by: Eric Auger 

---

v11 -> v12:
- reporting the addr associated with the fault and set the
  VIRTIO_IOMMU_FAULT_F_ADDRESS flag.
- added cpu_to_le

v10 -> v11:
- change a virtio_error into an error_report_once
  (no buffer available for output faults)
---
 hw/virtio/trace-events   |  1 +
 hw/virtio/virtio-iommu.c | 73 +---
 2 files changed, 69 insertions(+), 5 deletions(-)

diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
index 095aa8b509..e83500bee9 100644
--- a/hw/virtio/trace-events
+++ b/hw/virtio/trace-events
@@ -72,3 +72,4 @@ virtio_iommu_put_endpoint(uint32_t ep_id) "Free endpoint=%d"
 virtio_iommu_get_domain(uint32_t domain_id) "Alloc domain=%d"
 virtio_iommu_put_domain(uint32_t domain_id) "Free domain=%d"
 virtio_iommu_translate_out(uint64_t virt_addr, uint64_t phys_addr, uint32_t 
sid) "0x%"PRIx64" -> 0x%"PRIx64 " for sid=%d"
+virtio_iommu_report_fault(uint8_t reason, uint32_t flags, uint32_t endpoint, 
uint64_t addr) "FAULT reason=%d flags=%d endpoint=%d address =0x%"PRIx64
diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
index d192bcb505..09193970ee 100644
--- a/hw/virtio/virtio-iommu.c
+++ b/hw/virtio/virtio-iommu.c
@@ -477,6 +477,51 @@ out:
 }
 }
 
+static void virtio_iommu_report_fault(VirtIOIOMMU *viommu, uint8_t reason,
+  int flags, uint32_t endpoint,
+  uint64_t address)
+{
+VirtIODevice *vdev = >parent_obj;
+VirtQueue *vq = viommu->event_vq;
+struct virtio_iommu_fault fault;
+VirtQueueElement *elem;
+size_t sz;
+
+memset(, 0, sizeof(fault));
+fault.reason = reason;
+fault.flags = cpu_to_le32(flags);
+fault.endpoint = cpu_to_le32(endpoint);
+fault.address = cpu_to_le64(address);
+
+for (;;) {
+elem = virtqueue_pop(vq, sizeof(VirtQueueElement));
+
+if (!elem) {
+error_report_once(
+"no buffer available in event queue to report event");
+return;
+}
+
+if (iov_size(elem->in_sg, elem->in_num) < sizeof(fault)) {
+virtio_error(vdev, "error buffer of wrong size");
+virtqueue_detach_element(vq, elem, 0);
+g_free(elem);
+continue;
+}
+break;
+}
+/* we have a buffer to fill in */
+sz = iov_from_buf(elem->in_sg, elem->in_num, 0,
+  , sizeof(fault));
+assert(sz == sizeof(fault));
+
+trace_virtio_iommu_report_fault(reason, flags, endpoint, address);
+virtqueue_push(vq, elem, sz);
+virtio_notify(vdev, vq);
+g_free(elem);
+
+}
+
 static IOMMUTLBEntry virtio_iommu_translate(IOMMUMemoryRegion *mr, hwaddr addr,
 IOMMUAccessFlags flag,
 int iommu_idx)
@@ -485,9 +530,10 @@ static IOMMUTLBEntry 
virtio_iommu_translate(IOMMUMemoryRegion *mr, hwaddr addr,
 VirtIOIOMMUInterval interval, *mapping_key;
 VirtIOIOMMUMapping *mapping_value;
 VirtIOIOMMU *s = sdev->viommu;
+bool read_fault, write_fault;
 VirtIOIOMMUEndpoint *ep;
+uint32_t sid, flags;
 bool bypass_allowed;
-uint32_t sid;
 bool found;
 
 interval.low = addr;
@@ -513,6 +559,9 @@ static IOMMUTLBEntry 
virtio_iommu_translate(IOMMUMemoryRegion *mr, hwaddr addr,
 if (!ep) {
 if (!bypass_allowed) {
 error_report_once("%s sid=%d is not known!!", __func__, sid);
+virtio_iommu_report_fault(s, VIRTIO_IOMMU_FAULT_R_UNKNOWN,
+  VIRTIO_IOMMU_FAULT_F_ADDRESS,
+  sid, addr);
 } else {
 entry.perm = flag;
 }
@@ -524,6 +573,9 @@ static IOMMUTLBEntry 
virtio_iommu_translate(IOMMUMemoryRegion *mr, hwaddr addr,
 error_report_once("%s %02x:%02x.%01x not attached to any domain",
   __func__, PCI_BUS_NUM(sid),
   PCI_SLOT(sid), PCI_FUNC(sid));
+virtio_iommu_report_fault(s, VIRTIO_IOMMU_FAULT_R_DOMAIN,
+  VIRTIO_IOMMU_FAULT_F_ADDRESS,
+  sid, addr);
 } else {
 entry.perm = flag;
 }
@@ -536,15 +588,26 @@ static IOMMUTLBEntry 
virtio_iommu_translate(IOMMUMemoryRegion *mr, hwaddr addr,
 if (!found) {
 error_report_once("%s no mapping for 0x%"PRIx64" for sid=%d",
   __func__, addr, sid);
+virtio_iommu_report_fault(s, VIRTIO_IOMMU_FAULT_R_MAPPING,
+  VIRTIO_IOMMU_FAULT_F_ADDRESS,
+  sid, addr);
 goto unlock;
 }
 
-if (((flag & IOMMU_RO) &&
-!(mapping_value->flags & VIRTIO_IOMMU_MAP_F_READ)) ||
-((flag & IOMMU_WO) &&

Re: [PATCH v12 09/13] virtio-iommu: Implement fault reporting

Re: [PATCH v12 09/13] virtio-iommu: Implement fault reporting

Re: [PATCH v12 09/13] virtio-iommu: Implement fault reporting

Re: [PATCH v12 09/13] virtio-iommu: Implement fault reporting

[PATCH v12 09/13] virtio-iommu: Implement fault reporting

5 matches

Site Navigation

Mail list logo

Footer information