Re: [libvirt] [PATCHv4 3/5] S390: QEMU driver support for CCW addresses

2013-03-14 Thread Viktor Mihajlovski

On 03/14/2013 12:03 AM, Eric Blake wrote:

+if (virAsprintf(&addrstr, "%x.%x.%04x",


Should we zero-pad the first two fields too, or is it common
to only pad the last field in ccw addresses ?


but left this alone.  You can submit a followup patch if it is necessary.

That's OK, only the last field is zero-padded.



I'll push this, along with the other ACK'd patches in the series, shortly.


Thanks for the review, cleanup and pushing.

--

Mit freundlichen Grüßen/Kind Regards
   Viktor Mihajlovski

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Martina Köderitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCHv4 3/5] S390: QEMU driver support for CCW addresses

2013-03-13 Thread Eric Blake
On 03/13/2013 09:40 AM, Daniel P. Berrange wrote:
> On Tue, Mar 05, 2013 at 04:44:21PM +0100, Viktor Mihajlovski wrote:
>> This commit adds the QEMU driver support for CCW addresses. The
>> current QEMU only allows virtio devices to be attached to the
>> CCW bus. We named the new capability indicating that support
>> QEMU_CAPS_VIRTIO_CCW accordingly.
>>
>> The fact that CCW devices can only be assigned to domains with a
>> machine type of s390-ccw-virtio requires a few extra checks for
>> machine type in qemu_command.c on top of querying
>> QEMU_CAPS_VIRTIO_{CCW|S390}.
>>

>> +struct _qemuDomainCCWAddressSet {
>> +virHashTablePtr defined;
> 
> Too much whitespace   

I cleaned this up,

> 
>> +virDomainDeviceCCWAddress next;
>> +};
>> +
>> +static char*
>> +qemuCCWAddressAsString(virDomainDeviceCCWAddressPtr addr)
>> +{
>> +char *addrstr = NULL;
>> +
>> +if (virAsprintf(&addrstr, "%x.%x.%04x",
> 
> Should we zero-pad the first two fields too, or is it common
> to only pad the last field in ccw addresses ?

but left this alone.  You can submit a followup patch if it is necessary.

>> -static void
>> -qemuDomainAssignS390Addresses(virDomainDefPtr def, virQEMUCapsPtr qemuCaps)
>> +static int
>> +qemuDomainCCWAddressAllocate(virDomainDefPtr def ATTRIBUTE_UNUSED,
>> + virDomainDeviceDefPtr dev ATTRIBUTE_UNUSED,
>> + virDomainDeviceInfoPtr info,
>> + void * data)
>> +{
>> +return qemuDomainCCWAddressAssign(info,
>> +  (qemuDomainCCWAddressSetPtr)data,
> 
> You don't need to cast  'void *' in C.

I cleaned this up as well.

> ACK, since there's no bugs in my comments, just style issues.

I'll push this, along with the other ACK'd patches in the series, shortly.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCHv4 3/5] S390: QEMU driver support for CCW addresses

2013-03-13 Thread Daniel P. Berrange
On Tue, Mar 05, 2013 at 04:44:21PM +0100, Viktor Mihajlovski wrote:
> This commit adds the QEMU driver support for CCW addresses. The
> current QEMU only allows virtio devices to be attached to the
> CCW bus. We named the new capability indicating that support
> QEMU_CAPS_VIRTIO_CCW accordingly.
> 
> The fact that CCW devices can only be assigned to domains with a
> machine type of s390-ccw-virtio requires a few extra checks for
> machine type in qemu_command.c on top of querying
> QEMU_CAPS_VIRTIO_{CCW|S390}.
> 
> The majority of the new functions deals with CCW address generation
> and management.
> 
> Signed-off-by: Viktor Mihajlovski 
> ---
> V2 Changes
>  - rebase, mainly addressing the rename of qemuCapsXXX to virQEMUCapsXXX 
> 
> V3 Changes
>  - revert machine based capability detection
>  - check the machine type in conjunction with s390-specific capability
>tests in qemu_command.c
>  - remove useless paranoia check in qemu_command.c
> 
> V4 Changes
>  - replace CCW address field name 'schid' with 'devno'
> 
>  src/qemu/qemu_capabilities.c |7 +-
>  src/qemu/qemu_capabilities.h |1 +
>  src/qemu/qemu_command.c  |  279 
> +++---
>  src/qemu/qemu_command.h  |6 +
>  src/qemu/qemu_domain.c   |1 +
>  src/qemu/qemu_domain.h   |3 +
>  src/qemu/qemu_process.c  |3 +
>  7 files changed, 280 insertions(+), 20 deletions(-)
> 
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 40022c1..79cfdb3 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -210,6 +210,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
>  
>"rng-random", /* 130 */
>"rng-egd",
> +  "virtio-ccw"
>  );
>  
>  struct _virQEMUCaps {
> @@ -1318,6 +1319,7 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] 
> = {
>  { "usb-hub", QEMU_CAPS_USB_HUB },
>  { "ich9-ahci", QEMU_CAPS_ICH9_AHCI },
>  { "virtio-blk-s390", QEMU_CAPS_VIRTIO_S390 },
> +{ "virtio-blk-ccw", QEMU_CAPS_VIRTIO_CCW },
>  { "sclpconsole", QEMU_CAPS_SCLP_S390 },
>  { "lsi53c895a", QEMU_CAPS_SCSI_LSI },
>  { "virtio-scsi-pci", QEMU_CAPS_VIRTIO_SCSI_PCI },
> @@ -1338,7 +1340,6 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] 
> = {
>  { "rng-egd", QEMU_CAPS_OBJECT_RNG_EGD },
>  };
>  
> -
>  static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsVirtioBlk[] = {
>  { "multifunction", QEMU_CAPS_PCI_MULTIFUNCTION },
>  { "bootindex", QEMU_CAPS_BOOTINDEX },
> @@ -1393,6 +1394,10 @@ static struct virQEMUCapsObjectTypeProps 
> virQEMUCapsObjectProps[] = {
>ARRAY_CARDINALITY(virQEMUCapsObjectPropsVirtioBlk) },
>  { "virtio-net-pci", virQEMUCapsObjectPropsVirtioNet,
>ARRAY_CARDINALITY(virQEMUCapsObjectPropsVirtioNet) },
> +{ "virtio-blk-ccw", virQEMUCapsObjectPropsVirtioBlk,
> +  ARRAY_CARDINALITY(virQEMUCapsObjectPropsVirtioBlk) },
> +{ "virtio-net-ccw", virQEMUCapsObjectPropsVirtioNet,
> +  ARRAY_CARDINALITY(virQEMUCapsObjectPropsVirtioNet) },
>  { "virtio-blk-s390", virQEMUCapsObjectPropsVirtioBlk,
>ARRAY_CARDINALITY(virQEMUCapsObjectPropsVirtioBlk) },
>  { "virtio-net-s390", virQEMUCapsObjectPropsVirtioNet,
> diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
> index a895867..5c5dc5a 100644
> --- a/src/qemu/qemu_capabilities.h
> +++ b/src/qemu/qemu_capabilities.h
> @@ -171,6 +171,7 @@ enum virQEMUCapsFlags {
>  QEMU_CAPS_OBJECT_RNG_RANDOM  = 130, /* the rng-random backend for
> virtio rng */
>  QEMU_CAPS_OBJECT_RNG_EGD = 131, /* EGD protocol daemon for rng */
> +QEMU_CAPS_VIRTIO_CCW = 132, /* -device virtio-*-ccw */
>  
>  QEMU_CAPS_LAST,   /* this must always be the last item */
>  };
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 201fac1..cc64c17 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -799,6 +799,100 @@ no_memory:
>  return -1;
>  }
>  
> +/* S390 ccw bus support */
> +
> +struct _qemuDomainCCWAddressSet {
> +virHashTablePtr defined;

Too much whitespace   

> +virDomainDeviceCCWAddress next;
> +};
> +
> +static char*
> +qemuCCWAddressAsString(virDomainDeviceCCWAddressPtr addr)
> +{
> +char *addrstr = NULL;
> +
> +if (virAsprintf(&addrstr, "%x.%x.%04x",

Should we zero-pad the first two fields too, or is it common
to only pad the last field in ccw addresses ?

> +addr->cssid,
> +addr->ssid,
> +addr->devno) < 0) {
> +virReportOOMError();
> +return NULL;
> +}
> +
> +return addrstr;
> +}
> +

> -static void
> -qemuDomainAssignS390Addresses(virDomainDefPtr def, virQEMUCapsPtr qemuCaps)
> +static int
> +qemuDomainCCWAddressAllocate(virDomainDefPtr def ATTRIBUTE_UNUSED,
> +