Re: [PATCH v2 kvmtool 10/30] virtio/pci: Make memory and IO BARs independent

[Alexandru Elisei <alexandru.elisei@arm.com>]

Hi,

On 1/29/20 6:16 PM, Andre Przywara wrote:
> On Thu, 23 Jan 2020 13:47:45 +0000
> Alexandru Elisei <alexandru.elisei@arm.com> wrote:
>
> Hi,
>
>> From: Julien Thierry <julien.thierry@arm.com>
>>
>> Currently, callbacks for memory BAR 1 call the IO port emulation.  This
>> means that the memory BAR needs I/O Space to be enabled whenever Memory
>> Space is enabled.
>>
>> Refactor the code so the two type of  BARs are independent. Also, unify
>> ioport/mmio callback arguments so that they all receive a virtio_device.
> That's a nice cleanup, I like that it avoids shoehorning everything as legacy I/O into the emulation.
>
> Just a nit below, but nevertheless:
>  
>> Signed-off-by: Julien Thierry <julien.thierry@arm.com>
>> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Thank you!

[..]

>
>> +static bool virtio_pci__io_in(struct ioport *ioport, struct kvm_cpu *vcpu, u16 port, void *data, int size)
>> +{
>> +	unsigned long offset;
>> +	struct virtio_device *vdev;
>> +	struct virtio_pci *vpci;
>> +
>> +	vdev = ioport->priv;
>> +	vpci = vdev->virtio;
>> +	offset = port - vpci->port_addr;
> You could initialise the variables directly at their declaration, which looks nicer and underlines that they are just helper variables.
> Same below.

Sure, makes sense. I'll make the change for virtio_pci__io_{in,out}.

Thanks, Alex

>
> Cheers,
> Andre.
>
>> +
>> +	return virtio_pci__data_in(vcpu, vdev, offset, data, size);
>> +}
>> +
>>  static void update_msix_map(struct virtio_pci *vpci,
>>  			    struct msix_table *msix_entry, u32 vecnum)
>>  {
>> @@ -185,8 +195,8 @@ static void update_msix_map(struct virtio_pci *vpci,
>>  	irq__update_msix_route(vpci->kvm, gsi, &msix_entry->msg);
>>  }
>>  
>> -static bool virtio_pci__specific_io_out(struct kvm *kvm, struct virtio_device *vdev, u16 port,
>> -					void *data, int size, int offset)
>> +static bool virtio_pci__specific_data_out(struct kvm *kvm, struct virtio_device *vdev,
>> +					  void *data, int size, unsigned long offset)
>>  {
>>  	struct virtio_pci *vpci = vdev->virtio;
>>  	u32 config_offset, vec;
>> @@ -259,19 +269,16 @@ static bool virtio_pci__specific_io_out(struct kvm *kvm, struct virtio_device *v
>>  	return false;
>>  }
>>  
>> -static bool virtio_pci__io_out(struct ioport *ioport, struct kvm_cpu *vcpu, u16 port, void *data, int size)
>> +static bool virtio_pci__data_out(struct kvm_cpu *vcpu, struct virtio_device *vdev,
>> +				 unsigned long offset, void *data, int size)
>>  {
>> -	unsigned long offset;
>>  	bool ret = true;
>> -	struct virtio_device *vdev;
>>  	struct virtio_pci *vpci;
>>  	struct kvm *kvm;
>>  	u32 val;
>>  
>>  	kvm = vcpu->kvm;
>> -	vdev = ioport->priv;
>>  	vpci = vdev->virtio;
>> -	offset = port - vpci->port_addr;
>>  
>>  	switch (offset) {
>>  	case VIRTIO_PCI_GUEST_FEATURES:
>> @@ -304,13 +311,26 @@ static bool virtio_pci__io_out(struct ioport *ioport, struct kvm_cpu *vcpu, u16
>>  		virtio_notify_status(kvm, vdev, vpci->dev, vpci->status);
>>  		break;
>>  	default:
>> -		ret = virtio_pci__specific_io_out(kvm, vdev, port, data, size, offset);
>> +		ret = virtio_pci__specific_data_out(kvm, vdev, data, size, offset);
>>  		break;
>>  	};
>>  
>>  	return ret;
>>  }
>>  
>> +static bool virtio_pci__io_out(struct ioport *ioport, struct kvm_cpu *vcpu, u16 port, void *data, int size)
>> +{
>> +	unsigned long offset;
>> +	struct virtio_device *vdev;
>> +	struct virtio_pci *vpci;
>> +
>> +	vdev = ioport->priv;
>> +	vpci = vdev->virtio;
>> +	offset = port - vpci->port_addr;
>> +
>> +	return virtio_pci__data_out(vcpu, vdev, offset, data, size);
>> +}
>> +
>>  static struct ioport_operations virtio_pci__io_ops = {
>>  	.io_in	= virtio_pci__io_in,
>>  	.io_out	= virtio_pci__io_out,
>> @@ -320,7 +340,8 @@ static void virtio_pci__msix_mmio_callback(struct kvm_cpu *vcpu,
>>  					   u64 addr, u8 *data, u32 len,
>>  					   u8 is_write, void *ptr)
>>  {
>> -	struct virtio_pci *vpci = ptr;
>> +	struct virtio_device *vdev = ptr;
>> +	struct virtio_pci *vpci = vdev->virtio;
>>  	struct msix_table *table;
>>  	int vecnum;
>>  	size_t offset;
>> @@ -419,11 +440,15 @@ static void virtio_pci__io_mmio_callback(struct kvm_cpu *vcpu,
>>  					 u64 addr, u8 *data, u32 len,
>>  					 u8 is_write, void *ptr)
>>  {
>> -	struct virtio_pci *vpci = ptr;
>> -	int direction = is_write ? KVM_EXIT_IO_OUT : KVM_EXIT_IO_IN;
>> -	u16 port = vpci->port_addr + (addr & (PCI_IO_SIZE - 1));
>> +	struct virtio_device *vdev = ptr;
>> +	struct virtio_pci *vpci = vdev->virtio;
>>  
>> -	kvm__emulate_io(vcpu, port, data, direction, len, 1);
>> +	if (!is_write)
>> +		virtio_pci__data_in(vcpu, vdev, addr - vpci->mmio_addr,
>> +				    data, len);
>> +	else
>> +		virtio_pci__data_out(vcpu, vdev, addr - vpci->mmio_addr,
>> +				     data, len);
>>  }
>>  
>>  int virtio_pci__init(struct kvm *kvm, void *dev, struct virtio_device *vdev,
>> @@ -445,13 +470,13 @@ int virtio_pci__init(struct kvm *kvm, void *dev, struct virtio_device *vdev,
>>  
>>  	vpci->mmio_addr = pci_get_mmio_block(PCI_IO_SIZE);
>>  	r = kvm__register_mmio(kvm, vpci->mmio_addr, PCI_IO_SIZE, false,
>> -			       virtio_pci__io_mmio_callback, vpci);
>> +			       virtio_pci__io_mmio_callback, vdev);
>>  	if (r < 0)
>>  		goto free_ioport;
>>  
>>  	vpci->msix_io_block = pci_get_mmio_block(PCI_IO_SIZE * 2);
>>  	r = kvm__register_mmio(kvm, vpci->msix_io_block, PCI_IO_SIZE * 2, false,
>> -			       virtio_pci__msix_mmio_callback, vpci);
>> +			       virtio_pci__msix_mmio_callback, vdev);
>>  	if (r < 0)
>>  		goto free_mmio;
>>