> static const struct {
> guid_t guid;
> } vmbus_unsupported_devs[] = {
> @@ -198,6 +201,25 @@ static bool is_unsupported_vmbus_devs(const guid_t *guid)
> return false;
> }
>
> +static bool is_dev_hv_util(u16 dev_type)
> +{
> + switch (dev_type) {
> + case HV_SHUTDOWN:
> + fallthrough;
> + case HV_TS:
> + fallthrough;
> + case HV_HB:
> + fallthrough;
> + case HV_KVP:
> + fallthrough;
> + case HV_BACKUP:
> + return true;
> +
> + default:
> + return false;
> + }
> +}
> +
Rather than have a big case statement here, we already have
the "vmbus_devs" array that statically specifies various properties
of each VMBus device type. I'd suggest adding a field to those array
entries to indicate whether the device type is expected to be a
singleton. Then you can just do a direct lookup, like with the
"perf_device" and "allowed_in_isolated" properties.
> static u16 hv_get_dev_type(const struct vmbus_channel *channel)
> {
> const guid_t *guid = &channel->offermsg.offer.if_type;
> @@ -1004,6 +1026,26 @@ find_primary_channel_by_offer(const struct vmbus_channel_offer_channel *offer)
> return channel;
> }
>
> +static u16 vmbus_is_valid_hvutil_offer(const struct vmbus_channel_offer_channel *offer)
> +{
> + const guid_t *guid = &offer->offer.if_type;
> + u16 i;
> +
> + if (is_hvsock_offer(offer))
> + return HV_UNKNOWN;
> +
> + for (i = HV_IDE; i < HV_UNKNOWN; i++) {
> + if (guid_equal(guid, &vmbus_devs[i].guid) && is_dev_hv_util(i)) {
Ideally, we should avoid coding yet another case of searching through
the vmbus_devs[] array for a matching GUID. The function hv_get_dev_type()
already does this, and returns the index into the vmbus_devs[] array.
You could probably use that function, and then just pass the index as
the argument to this function.
That index is also stored as the "device_id" (arguably mis-named) in the
struct vmbus_channel, so it's already available in the rescind path.
> + if (refcount_read(&singleton_vmbus_devs[i]))
> + return HV_UNKNOWN;
> + refcount_set(&singleton_vmbus_devs[i], 1);
> + return i;
> + }
> + }
> +
> + return i;
> +}
> +
> static bool vmbus_is_valid_offer(const struct vmbus_channel_offer_channel *offer)
> {
> const guid_t *guid = &offer->offer.if_type;
> @@ -1031,6 +1073,7 @@ static void vmbus_onoffer(struct vmbus_channel_message_header *hdr)
> struct vmbus_channel_offer_channel *offer;
> struct vmbus_channel *oldchannel, *newchannel;
> size_t offer_sz;
> + u16 dev_type;
>
> offer = (struct vmbus_channel_offer_channel *)hdr;
>
> @@ -1115,11 +1158,29 @@ static void vmbus_onoffer(struct vmbus_channel_message_header *hdr)
> return;
> }
>
> + /*
> + * If vmbus_is_valid_offer() returns -
> + * HV_UNKNOWN - Subsequent offer received for hv_util dev, thus reject offer.
> + * HV_SHUTDOWN|HV_TS|HV_KVP|HV_HB|HV-KVP|HV_BACKUP - Increment refcount
> + * Others - Continue as is without doing anything.
> + *
> + * NOTE - We do not want to increase refcount if we resume from hibernation.
> + */
> + dev_type = vmbus_is_valid_hvutil_offer(offer);
> + if (dev_type == HV_UNKNOWN) {
> + pr_err_ratelimited("Invalid hv_util offer %d from the host supporting "
> + "isolation\n", offer->child_relid);
This check for multiple instances of a singleton device is not limited
to just CoCo VMs (a.k.a. "isolated VMs"). So the error message here really
shouldn't reference "host supporting isolation".
> + atomic_dec(&vmbus_connection.offer_in_progress);
> + return;
> + }
> +
> /* Allocate the channel object and save this offer. */
> newchannel = alloc_channel();
> if (!newchannel) {
> vmbus_release_relid(offer->child_relid);
> atomic_dec(&vmbus_connection.offer_in_progress);
> + if (is_dev_hv_util(dev_type))
> + refcount_dec(&singleton_vmbus_devs[dev_type]);
It might be good to have a function that combines the above two lines.
Then the two parallel functions are:
1) vmbus_is_valid_hvutil_offer() which marks a singleton device as
"already present" [and that function probably needs a new name]
2) vmbus_clear_singleton_device(), [or something similar] that clears
the boolean if it is a singleton device.
vmbus_clear_singleton_device() would also be used in the
rescind path and in the vmbus_add_channel_work() error path
that I mention below.
> pr_err("Unable to allocate channel object\n");
> return;
> }
There's another error case in the channel offer path that needs
to be handled. vmbus_add_channel_work() can fail, in which case
the new channel is cleaned up and removed. The accounting of
singleton devices must be updated if the channel is deleted via
this error path.
> @@ -1235,7 +1296,6 @@ static void vmbus_onoffer_rescind(struct vmbus_channel_message_header *hdr)
> /*
> * At this point, the rescind handling can proceed safely.
> */
> -
This is a spurious whitespace change that should be avoided.
> if (channel->device_obj) {
> if (channel->chn_rescind_callback) {
> channel->chn_rescind_callback(channel);
> @@ -1251,6 +1311,8 @@ static void vmbus_onoffer_rescind(struct vmbus_channel_message_header *hdr)
> */
> dev = get_device(&channel->device_obj->device);
> if (dev) {
> + if (is_dev_hv_util(hv_get_dev_type(channel)))
As noted above, the "dev_type" is already stored in the channel structure
as field "device_id" (which is a bit mis-named).
Michael
> + refcount_dec(&singleton_vmbus_devs[hv_get_dev_type(channel)]);
> vmbus_device_unregister(channel->device_obj);
> put_device(dev);
> }
