[Xen-devel] Re: [Qemu-devel] [PATCH RFC V1 01/11] Introduce HostPCIDevice to access a pci device on the host.

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Tue, Oct 4, 2011 at 19:21, Jan Kiszka <jan.kiszka@xxxxxx> wrote:
> This wasn't run through checkpatch.pl, I bet.
>
> On 2011-10-04 16:51, Anthony PERARD wrote:
>> Signed-off-by: Anthony PERARD <anthony.perard@xxxxxxxxxx>
>> ---
>> Âhw/host-pci-device.c | Â192 
>> ++++++++++++++++++++++++++++++++++++++++++++++++++
>> Âhw/host-pci-device.h | Â 36 +++++++++
>> Â2 files changed, 228 insertions(+), 0 deletions(-)
>> Âcreate mode 100644 hw/host-pci-device.c
>> Âcreate mode 100644 hw/host-pci-device.h
>>
>> diff --git a/hw/host-pci-device.c b/hw/host-pci-device.c
>> new file mode 100644
>> index 0000000..b3f2899
>> --- /dev/null
>> +++ b/hw/host-pci-device.c
>> @@ -0,0 +1,192 @@
>> +#include "qemu-common.h"
>> +#include "host-pci-device.h"
>> +
>> +static int path_to(const HostPCIDevice *d,
>> + Â Â Â Â Â Â Â Â Â const char *name, char *buf, ssize_t size)
>> +{
>> + Â Âreturn snprintf(buf, size, "/sys/bus/pci/devices/%04x:%02x:%02x.%x/%s",
>> + Â Â Â Â Â Â Â Â Â Âd->domain, d->bus, d->dev, d->func, name);
>> +}
>> +
>> +static int get_resource(HostPCIDevice *d)
>> +{
>> + Â Âint i;
>> + Â ÂFILE *f;
>> + Â Âchar path[PATH_MAX];
>> + Â Âunsigned long long start, end, flags, size;
>> +
>> + Â Âpath_to(d, "resource", path, sizeof (path));
>> + Â Âf = fopen(path, "r");
>> + Â Âif (!f) {
>> + Â Â Â Âfprintf(stderr, "Error: Can't open %s: %s\n", path, 
>> strerror(errno));
>> + Â Â Â Âreturn -1;
>> + Â Â}
>> +
>> + Â Âfor (i = 0; i < PCI_NUM_REGIONS; i++) {
>> + Â Â Â Âif (fscanf(f, "%llx %llx %llx", &start, &end, &flags) != 3) {
>> + Â Â Â Â Â Âfprintf(stderr, "Error: Syntax error in %s\n", path);
>> + Â Â Â Â Â Âbreak;
>> + Â Â Â Â}
>> + Â Â Â Âif (start) {
>> + Â Â Â Â Â Âsize = end - start + 1;
>> + Â Â Â Â} else {
>> + Â Â Â Â Â Âsize = 0;
>> + Â Â Â Â}
>> +
>> + Â Â Â Âflags &= 0xf;
>
> No magic numbers please.
>
> It also looks a bit strange to me: It's the resource type encoded in the
> second byte? Aren't you interested in it?

Actually, we are interessted to have the BAR with the different flags
(IO/MEM, prefetch, size) like the value in the config space. Because
the base_address value will be given to the VM (by the function
pt_bar_reg_read). But I can later merge the values (start | (flags &
~pci_base_address_io/mem_mask)), and have the right value to give
back.

So here, I will keep seperate the base address, and the flags.

>> +
>> + Â Â Â Âif (i < PCI_ROM_SLOT) {
>> + Â Â Â Â Â Âd->base_addr[i] = start | flags;
>> + Â Â Â Â Â Âd->size[i] = size;
>> + Â Â Â Â} else {
>> + Â Â Â Â Â Âd->rom_base_addr = start | flags;
>> + Â Â Â Â Â Âd->rom_size = size;
>> + Â Â Â Â}
>> + Â Â}
>> +
>> + Â Âfclose(f);
>> + Â Âreturn 0;
>> +}
>> +
>> +static unsigned long get_value(HostPCIDevice *d, const char *name)
>> +{
>> + Â Âchar path[PATH_MAX];
>> + Â ÂFILE *f;
>> + Â Âunsigned long value;
>> +
>> + Â Âpath_to(d, name, path, sizeof (path));
>> + Â Âf = fopen(path, "r");
>> + Â Âif (!f) {
>> + Â Â Â Âfprintf(stderr, "Error: Can't open %s: %s\n", path, 
>> strerror(errno));
>> + Â Â Â Âreturn -1;
>> + Â Â}
>> + Â Âif (fscanf(f, "%lx\n", &value) != 1) {
>> + Â Â Â Âfprintf(stderr, "Error: Syntax error in %s\n", path);
>> + Â Â Â Âvalue = -1;
>> + Â Â}
>> + Â Âfclose(f);
>> + Â Âreturn value;
>> +}
>> +
>> +static int pci_dev_is_virtfn(HostPCIDevice *d)
>> +{
>> + Â Âint rc;
>> + Â Âchar path[PATH_MAX];
>> + Â Âstruct stat buf;
>> +
>> + Â Âpath_to(d, "physfn", path, sizeof (path));
>> + Â Ârc = !stat(path, &buf);
>> +
>> + Â Âreturn rc;
>> +}
>> +
>> +static int host_pci_config_fd(HostPCIDevice *d)
>
> [ We will also need the reverse: pass in open file descriptors that
> HostPCIDevice should use. Can be added later. ]
>
>> +{
>> + Â Âchar path[PATH_MAX];
>> +
>> + Â Âif (d->config_fd < 0) {
>> + Â Â Â Âpath_to(d, "config", path, sizeof (path));
>> + Â Â Â Âd->config_fd = open(path, O_RDWR);
>> + Â Â Â Âif (d->config_fd < 0) {
>> + Â Â Â Â Â Âfprintf(stderr, "HostPCIDevice: Can not open '%s': %s\n",
>> + Â Â Â Â Â Â Â Â Â Âpath, strerror(errno));
>> + Â Â Â Â}
>> + Â Â}
>> + Â Âreturn d->config_fd;
>> +}
>> +static int host_pci_config_read(HostPCIDevice *d, int pos, void *buf, int 
>> len)
>> +{
>> + Â Âint fd = host_pci_config_fd(d);
>> + Â Âint res = 0;
>> +
>> + Â Âres = pread(fd, buf, len, pos);
>> + Â Âif (res < 0) {
>> + Â Â Â Âfprintf(stderr, "host_pci_config: read failed: %s (fd: %i)\n",
>> + Â Â Â Â Â Â Â Âstrerror(errno), fd);
>> + Â Â Â Âreturn -1;
>> + Â Â}
>> + Â Âreturn res;
>> +}
>> +static int host_pci_config_write(HostPCIDevice *d,
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â int pos, const void *buf, int len)
>> +{
>> + Â Âint fd = host_pci_config_fd(d);
>> + Â Âint res = 0;
>> +
>> + Â Âres = pwrite(fd, buf, len, pos);
>> + Â Âif (res < 0) {
>> + Â Â Â Âfprintf(stderr, "host_pci_config: write failed: %s\n",
>> + Â Â Â Â Â Â Â Âstrerror(errno));
>> + Â Â Â Âreturn -1;
>> + Â Â}
>> + Â Âreturn res;
>> +}
>> +
>> +uint8_t host_pci_read_byte(HostPCIDevice *d, int pos)
>> +{
>> + Âuint8_t buf;
>> + Âhost_pci_config_read(d, pos, &buf, 1);
>> + Âreturn buf;
>> +}
>> +uint16_t host_pci_read_word(HostPCIDevice *d, int pos)
>> +{
>> + Âuint16_t buf;
>> + Âhost_pci_config_read(d, pos, &buf, 2);
>> + Âreturn le16_to_cpu(buf);
>> +}
>> +uint32_t host_pci_read_long(HostPCIDevice *d, int pos)
>> +{
>> + Âuint32_t buf;
>> + Âhost_pci_config_read(d, pos, &buf, 4);
>> + Âreturn le32_to_cpu(buf);
>> +}
>> +int host_pci_read_block(HostPCIDevice *d, int pos, uint8_t *buf, int len)
>> +{
>> + Âreturn host_pci_config_read(d, pos, buf, len);
>> +}
>> +
>> +int host_pci_write_byte(HostPCIDevice *d, int pos, uint8_t data)
>> +{
>> + Âreturn host_pci_config_write(d, pos, &data, 1);
>> +}
>> +int host_pci_write_word(HostPCIDevice *d, int pos, uint16_t data)
>> +{
>> + Âreturn host_pci_config_write(d, pos, &data, 2);
>
> You adjust endianess on read, but not on write.

Will fix that.

>> +}
>> +int host_pci_write_long(HostPCIDevice *d, int pos, uint32_t data)
>> +{
>> + Âreturn host_pci_config_write(d, pos, &data, 4);
>> +}
>> +int host_pci_write_block(HostPCIDevice *d, int pos, uint8_t *buf, int len)
>> +{
>> + Âreturn host_pci_config_write(d, pos, buf, len);
>> +}
>> +
>> +HostPCIDevice *host_pci_device_get(uint8_t bus, uint8_t dev, uint8_t func)
>> +{
>> + Â ÂHostPCIDevice *d = NULL;
>> +
>> + Â Âd = g_new0(HostPCIDevice, 1);
>> +
>> + Â Âd->config_fd = -1;
>> + Â Âd->domain = 0;
>> + Â Âd->bus = bus;
>> + Â Âd->dev = dev;
>> + Â Âd->func = func;
>> +
>> + Â Âif (host_pci_config_fd(d) == -1)
>> + Â Â Â Âgoto error;
>> + Â Âif (get_resource(d) == -1)
>> + Â Â Â Âgoto error;
>> +
>> + Â Âd->vendor_id = get_value(d, "vendor");
>> + Â Âd->device_id = get_value(d, "device");
>> + Â Âd->is_virtfn = pci_dev_is_virtfn(d);
>> +
>> + Â Âreturn d;
>> +error:
>> + Â Âif (d->config_fd >= 0)
>> + Â Â Â Âclose(d->config_fd);
>> + Â Âg_free(d);
>> + Â Âreturn NULL;
>> +}
>> diff --git a/hw/host-pci-device.h b/hw/host-pci-device.h
>> new file mode 100644
>> index 0000000..0137507
>> --- /dev/null
>> +++ b/hw/host-pci-device.h
>> @@ -0,0 +1,36 @@
>> +#ifndef HW_HOST_PCI_DEVICE
>> +# Âdefine HW_HOST_PCI_DEVICE
>> +
>> +#include "pci.h"
>> +
>> +typedef struct HostPCIDevice {
>> + Â Âuint16_t domain;
>> + Â Âuint8_t bus;
>> + Â Âuint8_t dev;
>> + Â Âuint8_t func;
>> +
>> + Â Âuint16_t vendor_id;
>> + Â Âuint16_t device_id;
>> +
>> + Â Âpcibus_t base_addr[PCI_NUM_REGIONS - 1];
>> + Â Âpcibus_t size[PCI_NUM_REGIONS - 1];
>> + Â Âpcibus_t rom_base_addr;
>> + Â Âpcibus_t rom_size;
>
> Regions deserve their own type IMHO. In KVM we have
>
> typedef struct {
> Â Âint type; Â Â Â Â Â /* Memory or port I/O */
> Â Âint valid;
> Â Âuint32_t base_addr;
> Â Âuint32_t size; Â Â/* size of the region */
> Â Âint resource_fd;
> } PCIRegion;
>
> Should probably become HostPCIIORegion (vs. virtual PCIIORegion), and
> our field types need some cleanups.

I will do that, but I think to have only base_addr, size and flags.
flags will be the flags given by the sysfs resource file. Is that OK ?

>> +
>> + Â Âbool is_virtfn;
>> +
>> + Â Âint config_fd;
>> +} HostPCIDevice;
>> +
>> +HostPCIDevice *host_pci_device_get(uint8_t bus, uint8_t dev, uint8_t func);
>
> And what about some host_pci_device_put when we're done with it?

Will do it.

>> +
>> +uint8_t host_pci_read_byte(HostPCIDevice *d, int pos);
>> +uint16_t host_pci_read_word(HostPCIDevice *d, int pos);
>> +uint32_t host_pci_read_long(HostPCIDevice *d, int pos);
>> +int host_pci_read_block(HostPCIDevice *d, int pos, uint8_t *buf, int len);
>> +int host_pci_write_byte(HostPCIDevice *d, int pos, uint8_t data);
>> +int host_pci_write_word(HostPCIDevice *d, int pos, uint16_t data);
>> +int host_pci_write_long(HostPCIDevice *d, int pos, uint32_t data);
>> +int host_pci_write_block(HostPCIDevice *d, int pos, uint8_t *buf, int len);
>
> I think these should be analogous to our pci layer:
> host_pci_get/set_byte/word/long/quad.

Yes, I will change that.

> Looks like it's generally useful for KVM as well.

Thanks,

-- 
Anthony PERARD

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel